Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: undefined json keys as defaults when using ?columns and a Prefer header #2672

Merged
merged 9 commits into from
Mar 22, 2023

Conversation

steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Feb 19, 2023

Closes #1567.

(field-with_sep has default 1 on the following)

  • Allows bulk insert with defaults
http POST "localhost:3000/complex_items?columns=id,name,field-with_sep,arr_data" "Prefer: return=representation" <<JSON
[
  {"id": 4, "name": "Vier"},
  {"id": 5, "name": "Funf", "arr_data": null},
  {"id": 6, "name": "Sechs", "field-with_sep": 6, "arr_data": [1,2,3]}
]
JSON

[
    { "arr_data": null, "field-with_sep": 1, "id": 4, "name": "Vier", "settings": null },
    { "arr_data": null, "field-with_sep": 1, "id": 5, "name": "Funf", "settings": null },
    { "arr_data": [ 1, 2, 3 ], "field-with_sep": 6, "id": 6, "name": "Sechs", "settings": null }
]
  • Allows updating with defaults
http PATCH "/complex_items?id=eq.3&columns=name,field-with_sep" "Prefer: return=representation" <<JSON
{"name": "Tres"}
JSON


[ { "arr_data": [ 1, 2, 3 ], "field-with_sep": 1, "id": 3, "name": "Tres", "settings": { "foo": { "bar": "baz", "int": 1 } } } ]
  • It works on views as well assuming they have a default set up using: ALTER VIEW test.complex_items_view ALTER COLUMN name SET DEFAULT 'Default'

Pending

  • Optionally apply the feature with a Prefer header(since this new insert query is more expensive)
  • CHANGELOG

@steve-chavez steve-chavez changed the title feat: unset json keys as defaults on ?columns feat: undefined json keys as defaults when using ?columns Feb 19, 2023
@steve-chavez
Copy link
Member Author

It works on views as well assuming they have a default set up using: ALTER VIEW test.complex_items_view ALTER COLUMN name SET DEFAULT 'Default'

The above is not ideal. When omitting a column on a view INSERT:

INSERT INTO test.complex_items_view(id,name) VALUES (11, 'Eleven') RETURNING *;
 id |  name  | settings | arr_data | field-with_sep
----+--------+----------+----------+----------------
 11 | Eleven |          |          |              1

PostgreSQL correctly infers the field-with_sep default which is defined on the source table but when leaving an undefined key on the json we don't infer the default. I'm leaving this for a later enhancement.

@steve-chavez steve-chavez force-pushed the columns-defaults branch 2 times, most recently from 55f9854 to 0f906e8 Compare February 19, 2023 06:27
@steve-chavez
Copy link
Member Author

Check if it makes sense for upsert(might need additional logic)

So I was thinking that doing this:

(consider that id:3 already exists)

http POST "localhost:3000/complex_items?columns=id,name,field-with_sep,arr_data" "Prefer: return=representation, resolution=merge-duplicates" <<JSON
[
  {"id": 3, "name": "Drei"},
  {"id": 4, "name": "Vier"},
  {"id": 5, "name": "Funf", "arr_data": null},
  {"id": 6, "name": "Sechs", "field-with_sep": 6, "arr_data": [1,2,3]}
]
JSON

Would only UPDATE the name column for the id: 3 column, currently it updates the field-with_sep to the default and arr_data to null.

I was thinking to do this automatically but:

@steve-chavez
Copy link
Member Author

steve-chavez commented Feb 21, 2023

So I just made a pgbench test on the old insert query compared to the new one:

old

$ postgrest-with-postgresql-15 -f test/pgbench/fixtures.sql pgbench -T 30 -f test/pgbench/1567/old.sql

pgbench (15.1)
starting vacuum...pgbench: error: ERROR:  relation "pgbench_branches" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_tellers" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_history" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
end.
transaction type: test/pgbench/1567/old.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 30 s
number of transactions actually processed: 146236
number of failed transactions: 0 (0.000%)
latency average = 0.205 ms
initial connection time = 1.234 ms
tps = 4874.710773 (without initial connection time)

new

$ postgrest-with-postgresql-15 -f test/pgbench/fixtures.sql pgbench -T 30 -f test/pgbench/1567/new.sql

pgbench (15.1)
starting vacuum...pgbench: error: ERROR:  relation "pgbench_branches" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_tellers" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_history" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
end.
transaction type: test/pgbench/1567/old.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 30 s
number of transactions actually processed: 146236
number of failed transactions: 0 (0.000%)
latency average = 0.205 ms
initial connection time = 1.234 ms
tps = 4874.710773 (without initial connection time)
pgbench (15.1)
starting vacuum...pgbench: error: ERROR:  relation "pgbench_branches" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_tellers" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
pgbench: error: ERROR:  relation "pgbench_history" does not exist
pgbench: detail: (ignoring this error and continuing anyway)
end.
transaction type: test/pgbench/1567/new.sql
scaling factor: 1
query mode: simple
number of clients: 1
number of threads: 1
maximum number of tries: 1
duration: 30 s
number of transactions actually processed: 121645
number of failed transactions: 0 (0.000%)
latency average = 0.247 ms
initial connection time = 1.253 ms
tps = 4054.990532 (without initial connection time)

results

So there's 16.8% loss in perf.

Shortcut for tables that don't have defaults(since this new insert query is more expensive)

The above is adequate but I'm also thinking maybe we should add a new Prefer header for this feature.
Like Prefer: infer-defaults=true.

@steve-chavez
Copy link
Member Author

Like Prefer: infer-defaults=true.

The above wouldn't be right if this feature is JSON specific, it would have to be like:

  • Content-Type: application/vnd.pgrst.object+json; apply-defaults=true
  • Content-Type: application/vnd.pgrst.array+json; apply-defaults=true

Maybe it could be added to CSV as well though.. in which case the Prefer would not be incorrect. It's also less ergonomic to switch the whole content type for an optional behavior.

Also both options could be done.

@steve-chavez
Copy link
Member Author

So I've added a Prefer: defaults=apply/ignore header. Maybe to be more precise this should be Prefer: undefined-keys = apply-defaults/ignore-defaults .

@steve-chavez steve-chavez force-pushed the columns-defaults branch 3 times, most recently from ee2bd01 to 412b414 Compare March 9, 2023 20:35
@steve-chavez steve-chavez marked this pull request as ready for review March 9, 2023 20:36
@steve-chavez steve-chavez changed the title feat: undefined json keys as defaults when using ?columns feat: undefined json keys as defaults when using ?columns and Prefer header Mar 9, 2023
@steve-chavez steve-chavez changed the title feat: undefined json keys as defaults when using ?columns and Prefer header feat: undefined json keys as defaults when using ?columns and a Prefer header Mar 9, 2023
@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 9, 2023

Having to add both columns and the Prefer header also seems like a complicated interface. Maybe it won't matter later when we get rid of the Haskell json processing - since this new Prefer would also be applied when we don't use columns.

@steve-chavez
Copy link
Member Author

So since this feature requires jsonb, we no longer would be able to insert json with duplicates keys, if we were to switch completely to jsonb. This would be a breaking change.

To avoid this, I'm only switching to jsonb whenever the new Prefer header is used. I've also added tests on for this(on dc33357).

@steve-chavez
Copy link
Member Author

steve-chavez commented Mar 21, 2023

Having to add both columns and the Prefer header also seems like a complicated interface.

To avoid complicating the interface, I've enabled this Prefer header to work without ?columns. I see no harm in doing so. We can document that this is only required with ?columns.

Maybe to be more precise this should be Prefer: undefined-keys = apply-defaults/ignore-defaults .

Also I went ahead with the above change. Seems clearer.

@steve-chavez
Copy link
Member Author

Tried to appease codecov with the method mentioned on #2671, it cleared up one warning but not the other 2, no idea how to solve those.

Will leave it like that, they're only record fields which codecov doesn't understand.

@steve-chavez
Copy link
Member Author

Looking good to merge now.

@wolfgangwalther
Copy link
Member

The above wouldn't be right if this feature is JSON specific, it would have to be like:
[...]
Maybe it could be added to CSV as well though.. in which case the Prefer would not be incorrect.
[...]
Maybe to be more precise this should be Prefer: undefined-keys = apply-defaults/ignore-defaults .

I agree with putting it as a prefer header. But this header is still JSON specific. "undefined" is a concept in JSON, I think. What about something like Prefer: missing-values=apply-defaults|ignore-defaults or Prefer: missing=default|null. I actually like the latter more.

@steve-chavez
Copy link
Member Author

What about something like Prefer: missing-values=apply-defaults|ignore-defaults or Prefer: missing=default|null. I actually like the latter more.

Great idea! It looks more generic.

Doing that on #2723.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Updating Columns to Default Values
3 participants