fix: emit warning for legacy target names by steve-chavez · Pull Request #4905 · PostgREST/postgrest

steve-chavez · 2026-05-09T02:21:31Z

Reverts #4104 and eases transition towards the breaking change.

When the request with the deprecated syntax comes we log a warning, like so:

$ PGRST_LOG_LEVEL=info postgrest-with-pg-14 -f test/spec/fixtures/load.sql postgrest-run

$ curl 'localhost:3000/projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:clients(*)&tasks.order=name.asc&clients.id=eq.1'

08/May/2026:21:10:24 -0500: WARNING: Embedded resource was referenced by relation name even though it has an alias. This is deprecated and will stop working in a future release.
08/May/2026:21:10:24 -0500: Please update the filters that use `tasks` to `the_tasks`, `clients` to `clientes` in `GET /projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:client
s(*)&tasks.order=name.asc&clients.id=eq.1`
127.0.0.1 - postgrest_test_anonymous [08/May/2026:21:10:24 -0500] "GET /projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:clients(*)&tasks.order=name.asc&clients.id=eq.1 HTTP/1
.1" 200 146 "" "curl/7.81.0"

Note that:

The request is repeated on the WARNING message. This is because the error logs go to stderr and access/apache logs to stdout, we can't ensure they appear at the same time.
Includes all the alias that need to be corrected "tasks to the_tasks, clients to clientes"

TODO

Add a config that allows user to opt in to this breaking change and removes the warning. Let's have a single config for all cases that can come up in the future. Like server-legacy-features=target-names.
Tests. fix: unexpected results when embedding the same table twice #4104 didn't include a test that confirms old target names didn't work.
Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.
~~The warning only happens for log-level=warn, this requires Switch to log-level=warn as default #4904 to be effective.~~ Not planned anymore, see also: fix: emit warning for legacy target names #4905 (comment)

steve-chavez · 2026-05-09T02:33:40Z

+  , relSpread                  :: Maybe SpreadType
+  , relSelect                  :: [RelSelectField]
+  , depth                      :: Depth -- ^ used for aliasing
+  , relIsLegacyTargetNameMatch :: Bool  -- ^ used to ease migration into a new version with breaking change


Main idea is to add an attribute to ReadPlan, that is then aggregated in App.hs with legacyWarnings to generate the logs. That keeps the Plan module pure.

laurenceisla · 2026-05-10T20:42:31Z

Includes all the alias that need to be corrected "tasks to the_tasks, clients to clientes"

While working on building this warning, I noticed that we might avoid the breaking change if we look for the filter twice in the list of embeddings:

We check if the relation alias is present in the embeddings list and use the filter on that embedding if found
If not, then we make the search in the list again, but we look for relation names now

All tests pass when I try this, including the one that revealed the breaking change, but there could be outliers that may still show a breaking change so I'd have to look for them. I think it's worth the try, but before doing that @steve-chavez do you see it as a viable alternative against this whole breaking change warning stuff? (this would still be useful for future breaking changes though)

steve-chavez · 2026-05-10T20:56:30Z

@laurenceisla I'd say go ahead if you think it's doable without a breaking change (not sure if possible). But we do need to add loadtests for these alias cases to see if the double searching you propose won't cause a noticeable regression.

wolfgangwalther · 2026-05-11T04:11:34Z

I think we should do the breaking change at some point anyway. The previous behavior was just wrong - once you rename it via an alias, that's the name it should go by. If you want another release with a warning etc. I'd be OK with that. But I'd also be OK without that added complexity and the status quo (the breaking fix as it is currently done on main).

laurenceisla · 2026-05-11T14:36:35Z

(not sure if possible)
The previous behavior was just wrong

Right, after pondering it for some time, I believe the breaking change is inevitable. Will continue with the TODO list for this PR.

steve-chavez · 2026-05-20T21:38:04Z

Now that #4904 is closed. Can we still emit the WARNING: logs I mentioend above? I'd think so given it's critical for the administrator to somehow communicate this to clients.

@wolfgangwalther WDYT?

steve-chavez · 2026-05-21T19:34:40Z

@laurenceisla How about merging the first revert in another PR? I think we agree that it's not right to make the breaking change this way. Otherwise we risk this going into a new release somehow.

wolfgangwalther · 2026-05-21T19:39:03Z

Can we still emit the WARNING: logs I mentioend above? I'd think so given it's critical for the administrator to somehow communicate this to clients.

@wolfgangwalther WDYT?

As I wrote in an earlier comment, I am not opposed to that. I don't feel like it's necessary, but if you do, I'm ok with it.

I think we should not revert it - because we will need to do it anyway, for correctness. So we better deal with it in a way that makes you feel good about it.

laurenceisla

This should be ready for review now.

Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.

Do you mean to test how it's currently working on main and to check the impact of this PR?

wolfgangwalther · 2026-06-02T06:44:30Z

Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.

Do you mean to test how it's currently working on main and to check the impact of this PR?

Just a note: The tests for the loadtest are always taken from the head branch. I.e., if you add a test in this PR, you will run it on all 3 branches under test, so you don't need a separate PR to get results from the new test.

steve-chavez · 2026-06-02T18:39:38Z

Do you mean to test how it's currently working on main and to check the impact of this PR?

@laurenceisla I meant adding a new request like /x?select=alias:table(*)&alias.id=eq.1 (2 or 3 aliased filters would be good) in test/load/targets.http, to check if the additional logic we add to make the fix conditional has an impact on performance.

laurenceisla · 2026-06-03T17:05:27Z

I added a loadtest and here are the results: https://github.com/PostgREST/postgrest/actions/runs/26861796165. It cannot compare to 200 HTTP responses in main because that request fails in main. However, compared to 14.12 it appears to be a slight increase but I think it's negligible, right? (a couple to a few μs).

steve-chavez · 2026-06-03T19:17:35Z

However, compared to 14.12 it appears to be a slight increase but I think it's negligible, right? (a couple to a few μs).

Right, I think there's no other way anyway, we'll also remove this logic on the next major. Then the slight perf drop will go away.

@laurenceisla Could you rebase this PR so the revert and 533dec4 are no longer included? We don't need them right?

laurenceisla · 2026-06-04T01:16:42Z

@laurenceisla Could you rebase this PR so the revert and 533dec4 are no longer included? We don't need them right?

Correct. I found it difficult to separate the "new config" commit from the "emit warning" commit without the whole "revert"->"readd" thing, so I squashed them into a single one. In fact maybe all of the commits in this PR should be squashed when merged?

steve-chavez · 2026-06-04T17:05:02Z

@laurenceisla Yes, can you squash? 🙏 We've disabled squash/merge from github UI.

wolfgangwalther · 2026-06-04T17:10:56Z

+  o@LegacyTargetNameWarningObs {} ->
+    when (logLevel >= LogError) $ do
+      logWithZTime loggerState $ observationMessages o


Why are we logging a warning on log level error? That doesn't make sense.

@wolfgangwalther I thought you agreed above that this was fine?

I don't see any other way to communicate to an admin that some requests will fail on a next version. My read is that these are critical. Perhaps we should remove the WARNING: prefix?

I agreed to emitting a warning. But a warning is emitted on log-level=warn, ofc?

How can this thing be critical, if the request actually succeeds? It really is just a warning, nothing else.

But a warning is emitted on log-level=warn, ofc?

The point is to communicate to every user about the upcoming breaking change, if we cannot make log-level=warn the default (#4904), then this is not effective.

How can this thing be critical, if the request actually succeeds?

Because it will fail on a next postgREST upgrade.

What other options do we have to communicate this?

What other options do we have to communicate this?

The release notes are the canonical way to communicate this. You can hint in the release notes that this is deprecated now and if people are unsure whether they depend on it, they can enable log-level=warn to see the relevant warnings.

I don't think we should go at this assuming we can communicate this to users who do not look at the release notes at all. Those are very likely not going to look at their logs either, no matter on which level you log that stuff.

Or, to put differently: I regularly look at the release notes of things when I update. I only look at the logs when something is already broken.

I still think we should assume that if an admin has any intent of keeping their service up without throwing random errors, they will look at the changelog before updating.

But that doesn't consider users that start off with the new v16 version, they could inadvertently use the deprecated feature with the default configs and then be surprised with the break on the next major.

That's why the point here is to have a good default, which should be warn. Also note that PostgreSQL itself has a log_min_messages=warning by default (ref).

Edit: opened #5009

But that doesn't consider users that start off with the new v16 version, they could inadvertently use the deprecated feature with the default configs and then be surprised with the break on the next major.

That's why the point here is to have a good default, which should be warn. Also note that PostgreSQL itself has a log_min_messages=warning by default (ref).

Actually that argument means we should:

Have a config option to toggle between warning/error for this case. (I think we do)

Default this config option to "error" (I think we don't)

Then:

New users would not turn this on - and have no problem.

Existing users reading the changelog can have a smooth transition.

Existing users not reading the changelog will have obscure errors anyway - either now or on the next major, where the feature is removed.

Yeah, I agree.. otherwise we won't be helping users that start on the new version.

So to summarize:

The server WARNING: should appear on log-level=warn and url_use_legacy_target_names=true.

The client header Warning should appear when the config option is url_use_legacy_target_names=true. This disregarding the log-level config (which is only server side).

The url_use_legacy_target_names should be false by default.

Yes, agreed.

Even though the above proposal is "correct", I don't think it solves the problem.

On the default case we're essentially only leaving the Warning header as trace but how would users catch that some requests have the Warning header? For that they'd have to have sentry/otel on the frontend, so the requests are traced. But then again we don't have sentry/otel support.

steve-chavez · 2026-06-17T01:04:28Z

There's another way to communicate warnings to clients: #5009 (comment)

@laurenceisla Maybe you can show how that would look like? I believe we need to shorten the original message.

steve-chavez · 2026-06-19T20:31:40Z

@laurenceisla I think the message should be like:

Warning: 299 PostgRESTv16 "Embedded resource was referenced by relation name even though it has an alias. This is deprecated and will stop working on a next release. Update 'tasks' to 'the_tasks', 'clients' to 'clientes' in 'tasks.order=name.asc&clients.id=eq.1'"

The main change is that we need to compress the URL string a bit (removing the /path and ?select=), otherwise the header could be truncated.

laurenceisla · 2026-06-22T18:07:10Z

@laurenceisla I think the message should be like:

Warning: 299 PostgRESTv16 "Embedded resource was referenced by relation name even though it has an alias. This is deprecated and will stop working on a next release. Update 'tasks' to 'the_tasks', 'clients' to 'clientes' in 'tasks.order=name.asc&clients.id=eq.1'"

Ah sorry, I thought you meant a working example. Yes, it would look like that.

The main change is that we need to compress the URL string a bit (removing the /path and ?select=), otherwise the header could be truncated.

Maybe we could take out the URL string altogether and just mention Update ... to ... in the query string filters. or something similar. We're one filter away from the message truncating anyways (also if the column or alias have a long name).

steve-chavez · 2026-06-22T18:23:49Z

Maybe we could take tout the URL string altogether and just mention Update ... to ... in the query string filters. or something similar.

Yeah I think that should be fine. One question here, for the example above users also need to update tasks.order=name.asc right? That would not be a "filter", so we should call them another way?

laurenceisla · 2026-06-22T18:52:31Z

That would not be a "filter", so we should call them another way?

Right. One idea could be to just mention them all, like: "filters, orders and limits." Not sure of a word to include them all right now.

steve-chavez · 2026-06-22T18:55:08Z

like: "filters, orders and limits."

Agree, that sounds good enough.

Adds `url_use_legacy_target_names` config which is enabled by default. It logs a WARNING when target names are used in filters instead of its alias, e.g. `table?select=alias:target(*)&target.id=eq.1`. When it's disabled, the previous example will return an error. It will only succeed if the alias is used, and it won't log the warning anymore. This is DEPRECATED and the disabled behavior will be the default for future releases.

steve-chavez mentioned this pull request May 9, 2026

fix: unexpected results when embedding the same table twice #4104

Merged

steve-chavez commented May 9, 2026

View reviewed changes

steve-chavez mentioned this pull request May 9, 2026

Unexpected results when embedding and filtering the same table twice #4075

Open

wolfgangwalther reviewed May 24, 2026

View reviewed changes

Comment thread src/PostgREST/Config.hs Outdated

laurenceisla force-pushed the legacy-target-names branch 5 times, most recently from afd0eb5 to 78bf1e5 Compare June 1, 2026 23:30

laurenceisla reviewed Jun 1, 2026

View reviewed changes

Comment thread CHANGELOG.md

laurenceisla marked this pull request as ready for review June 1, 2026 23:51

steve-chavez commented Jun 2, 2026

View reviewed changes

Comment thread CHANGELOG.md

steve-chavez commented Jun 2, 2026

View reviewed changes

Comment thread CHANGELOG.md

laurenceisla force-pushed the legacy-target-names branch from 78bf1e5 to 2c00085 Compare June 3, 2026 03:25

laurenceisla force-pushed the legacy-target-names branch from 2c00085 to cbb508a Compare June 4, 2026 01:08

wolfgangwalther reviewed Jun 4, 2026

View reviewed changes

laurenceisla force-pushed the legacy-target-names branch from cbb508a to 894cccf Compare June 4, 2026 17:49

steve-chavez mentioned this pull request Jun 16, 2026

Stop logging 4xx errors on log-level=warn and make it the default #5009

Open

laurenceisla force-pushed the legacy-target-names branch from 894cccf to bab60c4 Compare June 17, 2026 13:39

laurenceisla marked this pull request as draft June 23, 2026 02:59

steve-chavez mentioned this pull request Jun 23, 2026

Auto-removal of schemas in db-schemas once DROP SCHEMA .. is done #4364

Open

laurenceisla force-pushed the legacy-target-names branch from e45dc84 to ecffd28 Compare June 24, 2026 03:02

Uh oh!

Conversation

steve-chavez commented May 9, 2026 • edited by laurenceisla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurenceisla commented May 10, 2026

Uh oh!

steve-chavez commented May 10, 2026

Uh oh!

wolfgangwalther commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurenceisla commented May 11, 2026

Uh oh!

steve-chavez commented May 20, 2026

Uh oh!

steve-chavez commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented May 21, 2026

Uh oh!

Uh oh!

laurenceisla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wolfgangwalther commented Jun 2, 2026

Uh oh!

Uh oh!

Uh oh!

steve-chavez commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurenceisla commented Jun 3, 2026

Uh oh!

steve-chavez commented Jun 3, 2026

Uh oh!

laurenceisla commented Jun 4, 2026

Uh oh!

steve-chavez commented Jun 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez commented Jun 17, 2026

Uh oh!

steve-chavez commented Jun 19, 2026

Uh oh!

laurenceisla commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-chavez commented Jun 22, 2026

Uh oh!

laurenceisla commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-chavez commented Jun 22, 2026

Uh oh!

steve-chavez commented May 9, 2026 •

edited by laurenceisla

Loading

wolfgangwalther commented May 11, 2026 •

edited

Loading

steve-chavez commented May 21, 2026 •

edited

Loading

steve-chavez commented Jun 2, 2026 •

edited

Loading

steve-chavez Jun 15, 2026 •

edited

Loading

laurenceisla commented Jun 22, 2026 •

edited

Loading

laurenceisla commented Jun 22, 2026 •

edited

Loading