Re: [HACKERS] pg_dump versus rules, once again

[Benedikt Grundmann]

On 30 December 2016 at 11:58, Benedikt Grundmann 
wrote:

>
> On 17 November 2016 at 03:45, Robert Haas  wrote:
>
>> On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane  wrote:
>> > Robert Haas  writes:
>> >> On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane  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.
>
>
N/m turns out that using pg_dump -t  isn't a good way to test if
the hack works because than it always does the good thing.


>
>> --
>> 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
>>
>
>

Re: [HACKERS] pg_dump versus rules, once again

[Benedikt Grundmann]

On 17 November 2016 at 03:45, Robert Haas  wrote:

> On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane  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.


> --
> 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
>

Re: [HACKERS] pg_dump versus rules, once again

[Robert Haas]

On Wed, Nov 16, 2016 at 10:14 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane  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

Re: [HACKERS] pg_dump versus rules, once again

[Tom Lane]

Robert Haas  writes:
> On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane  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

Re: [HACKERS] pg_dump versus rules, once again

[Robert Haas]

On Wed, Nov 16, 2016 at 10:00 PM, Tom Lane  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

Re: [HACKERS] pg_dump versus rules, once again

[Tom Lane]

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

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index b938d79..b89bd99 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*** RestoreArchive(Archive *AHX)
*** 521,527 
  		 * knows how to do it, without depending on
  		 * te->dropStmt; use that.  For other objects we need
  		 * to parse the command.
- 		 *
  		 */
  		if (strncmp(te->desc, "BLOB", 4) == 0)
  		{
--- 521,526 
*** RestoreArchive(Archive *AHX)
*** 529,538 
  		}
  		else
  		{
- 			char		buffer[40];
- 			char	   *mark;
  			char	   *dropStmt = pg_strdup(te->dropStmt);
! 			char	   *dropStmtPtr = dropStmt;
  			PQExpBuffer ftStmt = createPQExpBuffer();
  
  			/*
--- 528,535 
  		}
  		else
  		{
  			char	   *dropStmt = pg_strdup(te->dropStmt);
! 			char	   *dropStmtOrig = dropStmt;
  			PQExpBuffer ftStmt = createPQExpBuffer();
  
  			/*
*** RestoreArchive(Archive *AHX)
*** 549,566 
  			/*
  			 * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does
  			 * not support the IF EXISTS clause, and therefore
! 			 * we simply emit the original command for such
! 			 * objects. For other objects, we need to extract
! 			 * the first part of the DROP which includes the
! 			 * object type. Most of the time this matches
  			 * te->desc, so search for that; however for the
  			 * different kinds of CONSTRAINTs, we know to
  			 * search for hardcoded "DROP CONSTRAINT" instead.
  			 */
! 			if (strcmp(te->desc, "DEFAULT") == 0)
  appendPQExpBufferStr(ftStmt, dropStmt);
  			else
  			{
  if (strcmp(te->desc, "CONSTRAINT") == 0 ||
   strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
  	strcmp(te->desc, "FK CONSTRAINT") == 0)
--- 546,573 
  			/*
  			 * ALTER TABLE..ALTER COLUMN..DROP DEFAULT does
  			 * not support the IF EXISTS clause, and therefore
! 			 * we simply emit the original command for DEFAULT
! 			 * objects (modulo the adjustment made above).
! 			 *
! 			 * If we used CREATE OR REPLACE VIEW as a means of
! 			 * quasi-dropping an ON SELECT rule, that should
! 			 * be emitted unchanged as well.
! 			 *
! 			 * For other object types, we need to extract the
! 			 * first part of the DROP which includes the
! 			 * object type.  Most of the time this matches
  			 * te->desc, so search for that; however for the
  			 * different kinds of CONSTRAINTs, we know to
  			 * search for hardcoded "DROP CONSTRAINT" instead.
  			 */
! 			if (strcmp(te->desc, "DEFAULT") == 0 ||
! strncmp(dropStmt, "CREATE OR REPLACE VIEW", 22) == 0)
  appendPQExpBufferStr(ftStmt, dropStmt);
  			else
  			{
+ char		buffer[40];
+ char	   *mark;
+ 
  if (strcmp(te->desc, "CONSTRAINT") == 0 ||
   strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
  	strcmp(te->desc, "FK CONSTRAINT") == 0)
*** RestoreArchive(Archive *AHX)
*** 570,588 
  			 te->desc);
  
  mark = strstr(dropStmt, buffer);
- Assert(mark != NULL);
  
! *mark = '\0';
! appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
!   dropStmt, buffer,
!   mark + strlen(buffer));
  			}
  
  			ahprintf(AH, "%s", ftStmt->data);
  
  			destroyPQExpBuffer(ftStmt);
! 
! 			pg_free(dropStmtPtr);
  		}
  	}
  }
--- 577,604 
  			 te->desc);
  
  mark = strstr(dropStmt, buffer);
  
! if (mark)
! {
! 	*mark = '\0';
! 	appendPQExpBuffer(ftStmt, "%s%s IF EXISTS%s",
! 	  dropStmt, buffer,
! 	  mark + strlen(buffer));
! }
! else
! {
! 	/* complain and emit unmodified command */
! 	write_msg(modulename,
! 			  "WARNING: could not find where to insert IF EXISTS in statement \"%s\"\n",
! 			  dropStmtOrig);
! 

[HACKERS] pg_dump versus rules, once again

[Tom Lane]

I looked into the problem reported at
https://www.postgresql.org/message-id/b3690957-fd8c-6def-d3ec-e589887dd0f1%40codata.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