Thread: pgsql: Get rid of the "new" and "old" entries in a view's rangetable.
Get rid of the "new" and "old" entries in a view's rangetable. The rule system needs "old" and/or "new" pseudo-RTEs in rule actions that are ON INSERT/UPDATE/DELETE. Historically it's put such entries into the ON SELECT rules of views as well, but those are really quite vestigial. The only thing we've used them for is to carry the view's relid forward to AcquireExecutorLocks (so that we can re-lock the view to verify it hasn't changed before re-using a plan) and to carry its relid and permissions data forward to execution-time permissions checks. What we can do instead of that is to retain these fields of the RTE_RELATION RTE for the view even after we convert it to an RTE_SUBQUERY RTE. This requires a tiny amount of extra complication in the planner and AcquireExecutorLocks, but on the other hand we can get rid of the logic that moves that data from one place to another. The principal immediate benefit of doing this, aside from a small saving in the pg_rewrite data for views, is that these pseudo-RTEs no longer trigger ruleutils.c's heuristic about qualifying variable names when the rangetable's length is more than 1. That results in quite a number of small simplifications in regression test outputs, which are all to the good IMO. Bump catversion because we need to dump a few more fields of RTE_SUBQUERY RTEs. While those will always be zeroes anyway in stored rules (because we'd never populate them until query rewrite) they are useful for debugging, and it seems like we'd better make sure to transmit such RTEs accurately in plans sent to parallel workers. I don't think the executor actually examines these fields after startup, but someday it might. This is a second attempt at committing 1b4d280ea. The difference from the first time is that now we can add some filtering rules to AdjustUpgrade.pm to allow cross-version upgrade testing to pass despite all the cosmetic changes in CREATE VIEW outputs. Amit Langote (filtering rules by me) Discussion: https://postgr.es/m/CA+HiwqEf7gPN4Hn+LoZ4tP2q_Qt7n3vw7-6fJKOf92tSEnX6Gg@mail.gmail.com Discussion: https://postgr.es/m/891521.1673657296@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/47bb9db75996232ea71fc1e1888ffb0e70579b54 Modified Files -------------- contrib/postgres_fdw/expected/postgres_fdw.out | 16 +- src/backend/commands/lockcmds.c | 9 - src/backend/commands/view.c | 107 ---- src/backend/nodes/outfuncs.c | 7 +- src/backend/nodes/readfuncs.c | 7 +- src/backend/optimizer/plan/setrefs.c | 26 +- src/backend/parser/parse_relation.c | 2 +- src/backend/rewrite/rewriteDefine.c | 7 - src/backend/rewrite/rewriteHandler.c | 38 +- src/backend/utils/cache/plancache.c | 3 +- src/bin/pg_dump/t/002_pg_dump.pl | 12 +- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 18 +- src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm | 133 ++++ src/test/regress/expected/aggregates.out | 26 +- src/test/regress/expected/alter_table.out | 16 +- src/test/regress/expected/collate.icu.utf8.out | 24 +- src/test/regress/expected/collate.linux.utf8.out | 24 +- src/test/regress/expected/collate.out | 26 +- src/test/regress/expected/compression.out | 4 +- src/test/regress/expected/create_view.out | 222 +++---- src/test/regress/expected/expressions.out | 24 +- src/test/regress/expected/groupingsets.out | 20 +- src/test/regress/expected/limit.out | 24 +- src/test/regress/expected/matview.out | 24 +- src/test/regress/expected/polymorphism.out | 8 +- src/test/regress/expected/rangefuncs.out | 34 +- src/test/regress/expected/rules.out | 748 +++++++++++------------ src/test/regress/expected/tablesample.out | 4 +- src/test/regress/expected/triggers.out | 4 +- src/test/regress/expected/updatable_views.out | 78 +-- src/test/regress/expected/window.out | 56 +- src/test/regress/expected/with.out | 32 +- src/test/regress/expected/xml.out | 6 +- src/test/regress/expected/xml_2.out | 6 +- 35 files changed, 907 insertions(+), 890 deletions(-)
Hi, On 2023-01-18 18:24:45 +0000, Tom Lane wrote: > Get rid of the "new" and "old" entries in a view's rangetable. This seems to have caused a test failure on windows. Looks like it's not indicating a real problem, just that collate.windows.win1252.out needs to be adapted. Here's the regression.diffs: https://api.cirrus-ci.com/v1/artifact/task/6607289507905536/testrun/build/testrun/regress/regress/regression.diffs Full test: https://cirrus-ci.com/task/6607289507905536 The other failures are just due to the main regression tests being "embedded". Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2023-01-18 18:24:45 +0000, Tom Lane wrote:
>> Get rid of the "new" and "old" entries in a view's rangetable.
> This seems to have caused a test failure on windows. Looks like it's not
> indicating a real problem, just that collate.windows.win1252.out needs to be
> adapted.
Huh, I wonder why the cfbot never complained about that when
it was testing this patch ... maybe it doesn't use win1252?
Will fix, thanks for letting me know.
regards, tom lane
Hi, On 2023-01-18 15:08:38 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-01-18 18:24:45 +0000, Tom Lane wrote: > >> Get rid of the "new" and "old" entries in a view's rangetable. > > > This seems to have caused a test failure on windows. Looks like it's not > > indicating a real problem, just that collate.windows.win1252.out needs to be > > adapted. > > Huh, I wonder why the cfbot never complained about that when > it was testing this patch ... maybe it doesn't use win1252? This is the same environment that cfbot is also using, so this is odd. And cfbot has been failing on windows since the commit went into master: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/ So I guess something must have changed in the patch since then? I guess it could be that something changed on the VM, but I don't think so... Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2023-01-18 15:08:38 -0500, Tom Lane wrote: >> Huh, I wonder why the cfbot never complained about that when >> it was testing this patch ... maybe it doesn't use win1252? > This is the same environment that cfbot is also using, so this is odd. And > cfbot has been failing on windows since the commit went into master: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/ > So I guess something must have changed in the patch since then? I guess it > could be that something changed on the VM, but I don't think so... Hmmm ... the collate.windows.win1252 test did not exist at all before 3-Jan (bf03cfd16), while 1b4d280ea went in on 11-Jan. So perhaps the most plausible theory is that the cfbot didn't test this patch between those dates. It should certainly have noticed if it did run it ... regards, tom lane
On Wed, Jan 18, 2023 at 03:35:41PM -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2023-01-18 15:08:38 -0500, Tom Lane wrote: > >> Huh, I wonder why the cfbot never complained about that when > >> it was testing this patch ... maybe it doesn't use win1252? > > > This is the same environment that cfbot is also using, so this is odd. And > > cfbot has been failing on windows since the commit went into master: > > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/ > > > So I guess something must have changed in the patch since then? I guess it > > could be that something changed on the VM, but I don't think so... > > Hmmm ... the collate.windows.win1252 test did not exist at all before > 3-Jan (bf03cfd16), while 1b4d280ea went in on 11-Jan. So perhaps the > most plausible theory is that the cfbot didn't test this patch between > those dates. It should certainly have noticed if it did run it ... Looks like it failed the last 7 days https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/41/4048 I also saw that it failed in its previous incarnation, before the revert, but didn't dig into it at that point.
On Wed, Jan 18, 2023 at 02:40:06PM -0600, Justin Pryzby wrote: > On Wed, Jan 18, 2023 at 03:35:41PM -0500, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > On 2023-01-18 15:08:38 -0500, Tom Lane wrote: > > >> Huh, I wonder why the cfbot never complained about that when > > >> it was testing this patch ... maybe it doesn't use win1252? > > > > > This is the same environment that cfbot is also using, so this is odd. And > > > cfbot has been failing on windows since the commit went into master: > > > > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/ > > > > > So I guess something must have changed in the patch since then? I guess it > > > could be that something changed on the VM, but I don't think so... > > > > Hmmm ... the collate.windows.win1252 test did not exist at all before > > 3-Jan (bf03cfd16), while 1b4d280ea went in on 11-Jan. So perhaps the > > most plausible theory is that the cfbot didn't test this patch between > > those dates. It should certainly have noticed if it did run it ... > > Looks like it failed the last 7 days > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/41/4048 Correction. It failed in the most recent 7 runs, which seem to be ~13 days ago through ~7 days ago, when its cfapp record was closed. 2023-01-12 01:06:50 Tom Lane (tgl) Closed in commitfest 2023-01 with status: Committed > I also saw that it failed in its previous incarnation, before the > revert, but didn't dig into it at that point.
Justin Pryzby <pryzby@telsasoft.com> writes: > On Wed, Jan 18, 2023 at 02:40:06PM -0600, Justin Pryzby wrote: >> Looks like it failed the last 7 days >> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/41/4048 > Correction. It failed in the most recent 7 runs, which seem to be ~13 > days ago through ~7 days ago, when its cfapp record was closed. Hmph. So I guess I just neglected to recheck the cfbot in the last few days before pushing the patch, or didn't look closely enough to see that there was a persistent problem (there are some transient failures in that series too). Ah well, it's only a minor issue. regards, tom lane