pg_dump versus rules, once again
I looked into the problem reported at
/messages/by-id/b3690957-fd8c-6def-d3ec-e589887dd0f1@codata.eu
It's easy to reproduce. Given this simple database:
create table tt (f1 int primary key, f2 text);
create view vv as select * from tt group by f1;
pg_dump with the --clean option will generate
DROP RULE "_RETURN" ON public.vv;
which the backend rejects:
ERROR: cannot drop rule _RETURN on view vv because view vv requires it
HINT: You can drop view vv instead.
The reason for this is that because the view is dependent on tt's primary
key constraint (since it omits an otherwise-required "GROUP BY f2"),
pg_dump has a circular dependency to deal with: it wants to create the
view pre-data, but the view definition won't work until after the pkey has
been created post-data.
Our longtime solution to circularities involving views is to break the
view into CREATE TABLE and then CREATE RULE "_RETURN", exploiting a
horribly ancient backwards-compatibility hack in the backend that will
turn an empty table into a view if it gets a command to create an ON
SELECT rule for it. That's fine until you add --clean to the mix, which
causes pg_dump to blindly emit a DROP RULE and then DROP TABLE. Lose.
One way to fix this would be to add code to the backend so that
DROP RULE "_RETURN" converts the view back into a table, but ick.
We've talked before about replacing this _RETURN-rule business with
CREATE OR REPLACE VIEW, ie the idea would be for pg_dump to first emit
a dummy view with the right column names/types, say
CREATE VIEW vv AS SELECT null::int AS f1, null::text AS f2;
and then later when it's safe, emit CREATE OR REPLACE VIEW with the view's
real query. The main point of this according to past discussion would be
to eliminate dump files' dependency on the _RETURN-rule implementation of
views, so that someday in the far future we could change that if we
wished. However, if that were how pg_dump dealt with circular view
dependencies, then it would not take much more code to emit
CREATE OR REPLACE VIEW vv AS SELECT null::int AS f1, null::text AS f2;
as a substitute for the DROP RULE "_RETURN" step in a --clean script.
(Later, after we'd gotten rid of whatever was circularly depending on
that, we would emit DROP VIEW vv.)
So I'm thinking of going and doing this. Any objections?
Although this is a bug fix, I'm not sure whether to consider
back-patching. The case isn't that hard to work around -- either ignore
the error, or change your view to spell out its GROUP BY in full.
But it'd be annoying to hit this during pg_upgrade for instance.
CREATE OR REPLACE VIEW has existed since 7.3, so we're not creating much
of a portability problem at the server end if we make this change.
However, I notice that the kluge that was added to RestoreArchive() for
--if-exists will dump core (Assert failure or null pointer dereference)
if an archived dropStmt isn't what it expects. I think that's broken
anyway, but it'd become actively broken as soon as we start handling views
this way, so we'd need to back-patch at least some change there.
Probably it's sufficient to teach that code to do nothing to statements
it doesn't recognize.
BTW, the ability to create a view that has this hazard has existed since
9.1.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
We've talked before about replacing this _RETURN-rule business with
CREATE OR REPLACE VIEW, ie the idea would be for pg_dump to first emit
a dummy view with the right column names/types, say
CREATE VIEW vv AS SELECT null::int AS f1, null::text AS f2;
and then later when it's safe, emit CREATE OR REPLACE VIEW with the view's
real query.
Here's a proposed patch for that. It turns out there are some other
kluges that can be gotten rid of while we're at it: no longer need the
idea of reloptions being attached to a rule, for instance.
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions. We could possibly
put the rest only into HEAD; I remain of two minds about that.
regards, tom lane
Attachments:
dump-view-fix.patchtext/x-diff; charset=us-ascii; name=dump-view-fix.patchDownload+206-177
On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.
Even if you patch future minor releases, past minor releases are still
going to exist out there in the wild for a long, long time.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.
Even if you patch future minor releases, past minor releases are still
going to exist out there in the wild for a long, long time.
Yeah, but it would only matter if you try to use pg_restore --clean --if-exists
with an archive file that happens to contain a view that has this issue.
Such cases would previously have failed anyway, because of precisely
the bug at issue ... and there aren't very many of them, or we'd have
noticed the problem before. So I don't feel *too* bad about this,
I just want to make sure we have a solution available.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.Even if you patch future minor releases, past minor releases are still
going to exist out there in the wild for a long, long time.Yeah, but it would only matter if you try to use pg_restore --clean --if-exists
with an archive file that happens to contain a view that has this issue.
Such cases would previously have failed anyway, because of precisely
the bug at issue ... and there aren't very many of them, or we'd have
noticed the problem before. So I don't feel *too* bad about this,
I just want to make sure we have a solution available.
Right, OK.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 17 November 2016 at 03:45, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.Even if you patch future minor releases, past minor releases are still
going to exist out there in the wild for a long, long time.Yeah, but it would only matter if you try to use pg_restore --clean
--if-exists
with an archive file that happens to contain a view that has this issue.
Such cases would previously have failed anyway, because of precisely
the bug at issue ... and there aren't very many of them, or we'd have
noticed the problem before. So I don't feel *too* bad about this,
I just want to make sure we have a solution available.Right, OK.
For what it is worth we just run into this problem on our postgres 9.2.17
installation on a hunch we (after reading Tom's initial email replaced the
view that caused this by this
create view ... as select * from (...original view definition...)
hack_around_pg_dump_versus_rules_bug;
Which caused pg_dump to change its behavior and instead emit create view
.... which is what we wanted (because we take filtered down and dependency
ordered outputs of pg_dump as the starting point for new patches to the
db). But it surprised me mildly that the hack "worked" so I thought I
would mention it here. It might just mean that I'm misunderstanding the
bug but if there was really a dependency in the original that dependency
still exists now.
Show quoted text
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 30 December 2016 at 11:58, Benedikt Grundmann <bgrundmann@janestreet.com>
wrote:
On 17 November 2016 at 03:45, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The changes in pg_backup_archiver.c would have to be back-patched
into all versions supporting --if-exists, so that they don't fail
on dump archives produced by patched versions.Even if you patch future minor releases, past minor releases are still
going to exist out there in the wild for a long, long time.Yeah, but it would only matter if you try to use pg_restore --clean
--if-exists
with an archive file that happens to contain a view that has this issue.
Such cases would previously have failed anyway, because of precisely
the bug at issue ... and there aren't very many of them, or we'd have
noticed the problem before. So I don't feel *too* bad about this,
I just want to make sure we have a solution available.Right, OK.
For what it is worth we just run into this problem on our postgres 9.2.17
installation on a hunch we (after reading Tom's initial email replaced the
view that caused this by thiscreate view ... as select * from (...original view definition...)
hack_around_pg_dump_versus_rules_bug;Which caused pg_dump to change its behavior and instead emit create view
.... which is what we wanted (because we take filtered down and dependency
ordered outputs of pg_dump as the starting point for new patches to the
db). But it surprised me mildly that the hack "worked" so I thought I
would mention it here. It might just mean that I'm misunderstanding the
bug but if there was really a dependency in the original that dependency
still exists now.
N/m turns out that using pg_dump -t <viewname> isn't a good way to test if
the hack works because than it always does the good thing.
Show quoted text
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers