Change in "policy" on dump ordering?

On 2/21/17 14:58, Jim Nasby wrote:

AFAICT in older versions only object types that absolutely had to wait
for DO_POST_DATA_BOUNDARY would do so. More recently though, objects are
being added after that (presumably because it's easier than renumbering
everything in dbObjectTypePriority).

Is there any specific assignment that you have concerns about?

Is this change a good or bad idea? Should there be an official guide for
where new things go?

The comment above dbObjectTypePriority explains it, doesn't it?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/21/17 4:25 PM, Peter Eisentraut wrote:

On 2/21/17 14:58, Jim Nasby wrote:

AFAICT in older versions only object types that absolutely had to wait
for DO_POST_DATA_BOUNDARY would do so. More recently though, objects are
being added after that (presumably because it's easier than renumbering
everything in dbObjectTypePriority).

Is there any specific assignment that you have concerns about?

Originally, no, but reviewing the list again I'm kindof wondering about
DO_DEFAULT_ACL, especially since the acl code in pg_dump looks at
defaults as part of what removes the need to explicitly dump
permissions. I'm also wondering if DO_POLICY could potentially affect
matviews?

Actually, I think matviews really need to be the absolute last thing.
What if you had a matview that referenced publications or subscriptions?
I'm guessing that would be broken right now.

Is this change a good or bad idea? Should there be an official guide for
where new things go?

The comment above dbObjectTypePriority explains it, doesn't it?

Not really; it just makes reference to needing to be in-sync with
pg_dump.c. My concern is that clearly people went to lengths in the past
to put everything possible before DO_PRE_DATA_BOUNDARY (ie, text search
and FDW) but most recently added stuff has gone after
DO_POST_DATA_BOUNDARY, even though there's no reason it couldn't be
pre-data. That's certainly a change, and I suspect it's not intentional
(other than it's obviously less work to stick stuff at the end, but that
could be fixed by having an array of the actual enum values and just
having pg_dump sort that when it starts).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 00:55, Jim Nasby wrote:

Originally, no, but reviewing the list again I'm kindof wondering about
DO_DEFAULT_ACL, especially since the acl code in pg_dump looks at
defaults as part of what removes the need to explicitly dump
permissions. I'm also wondering if DO_POLICY could potentially affect
matviews?

I'm not sure about the details of these, but I know that there are
reasons why the permissions stuff is pretty late in the dump in general.

Actually, I think matviews really need to be the absolute last thing.
What if you had a matview that referenced publications or subscriptions?
I'm guessing that would be broken right now.

I'm not sure what you have in mind here. Publications and subscriptions
don't interact with materialized views, so the relative order doesn't
really matter.

Not really; it just makes reference to needing to be in-sync with
pg_dump.c. My concern is that clearly people went to lengths in the past
to put everything possible before DO_PRE_DATA_BOUNDARY (ie, text search
and FDW) but most recently added stuff has gone after
DO_POST_DATA_BOUNDARY, even though there's no reason it couldn't be
pre-data. That's certainly a change, and I suspect it's not intentional

I think the recent additions actually were intentional, although one
could debate the intentions. ;-)

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 8:00 AM, Peter Eisentraut wrote:

Actually, I think matviews really need to be the absolute last thing.
What if you had a matview that referenced publications or subscriptions?
I'm guessing that would be broken right now.

I'm not sure what you have in mind here. Publications and subscriptions
don't interact with materialized views, so the relative order doesn't
really matter.

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was added
in 9.5. So if we want to treat this as a bug, they'd need to be patched
as well, which is a simple matter of swapping 33 and 34.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

Jim,

* Jim Nasby (Jim.Nasby@BlueTreble.com) wrote:

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was
added in 9.5. So if we want to treat this as a bug, they'd need to
be patched as well, which is a simple matter of swapping 33 and 34.

Can you clarify what misbehavior there is with RLS that would warrent
this being a bug..? I did consider where in the dump I thought policies
should go, though I may certainly have overlooked something.

Thanks!

Stephen

Hi,

On Wed, Feb 22, 2017 at 05:24:49PM -0600, Jim Nasby wrote:

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

Glad to hear - I vaguely remember this coming up in a different thread
some time ago, and I though you (Peter) had reservations about moving
things past after the ACL step, but I couldn't find this thread now
anymore, only
/messages/by-id/11166.1424357659@sss.pgh.pa.us

Patches for head attached.

Yay.

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index ea643397ba..708a47f3cb 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
* Sort priority for database object types.
* Objects are sorted by type, and within a type by name.
*
+ * Because materialized views can potentially reference system views,
+ * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ *

I think this comment is overly specific: any materialized view that
references a view or table in a different schema (pg_catalog or not)
will likely not refresh on pg_restore AIUI, so singling out system views
doesn't look right to me.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 5:38 PM, Michael Banck wrote:

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index ea643397ba..708a47f3cb 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -26,6 +26,9 @@ static const char *modulename = gettext_noop("sorter");
* Sort priority for database object types.
* Objects are sorted by type, and within a type by name.
*
+ * Because materialized views can potentially reference system views,
+ * DO_REFRESH_MATVIEW should always be the last thing on the list.
+ *

I think this comment is overly specific: any materialized view that
references a view or table in a different schema (pg_catalog or not)
will likely not refresh on pg_restore AIUI, so singling out system views
doesn't look right to me.

This isn't a matter of excluded schemas. The problem is that if you had
a matview that referenced a system view for something that was restored
after DO_REFRESH_MATVIEW (such as subscriptions) then the view would be
inaccurate after the restore.

Stephen, hopefully that answers your question as well. :)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

I've found the (AIUI) previous discussion about this, it's Bug #13907:
/messages/by-id/20160202161407.2778.24659@wrigleys.postgresql.org

On Wed, Feb 22, 2017 at 05:24:49PM -0600, Jim Nasby wrote:

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

In /messages/by-id/9af4bc32-7e55-a21d-47e7-608582a8c48d@2ndquadrant.com
you (Peter) wrote:

"The reason that ACLs are restored last is that they could contain owner
self-revokes. So anything that you run after the ACLs could fail
because of that. I think a more complete fix would be to split up the
ACL restores into the general part, which you would run right after the
object is restored, and the owner revokes, which you would run last."

Patches for head attached.

FWIW, Keven Grittner had proposed a more involved patch in
/messages/by-id/CACjxUsNmpQDL58zRm3EFS9atqGT8+X_2+FOCXpYBwWZw5wgi-A@mail.gmail.com

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/22/17 18:24, Jim Nasby wrote:

On 2/22/17 12:29 PM, Peter Eisentraut wrote:

On 2/22/17 10:14, Jim Nasby wrote:

CREATE MATERIALIZED VIEW tmv AS SELECT * FROM pg_subscription;
SELECT 0

IOW, you can create matviews that depend on any other
table/view/matview, but right now if the matview includes certain items
it will mysteriously end up empty post-restore.

Yes, by that logic matview refresh should always be last.

Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was added
in 9.5. So if we want to treat this as a bug, they'd need to be patched
as well, which is a simple matter of swapping 33 and 34.

I wonder whether we should emphasize this change by assigning
DO_REFRESH_MATVIEW a higher number, like 100?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/1/17 08:36, Peter Eisentraut wrote:

On 2/22/17 18:24, Jim Nasby wrote:

Yes, by that logic matview refresh should always be last.

Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was added
in 9.5. So if we want to treat this as a bug, they'd need to be patched
as well, which is a simple matter of swapping 33 and 34.

I wonder whether we should emphasize this change by assigning
DO_REFRESH_MATVIEW a higher number, like 100?

Since there wasn't any interest in that idea, I have committed Jim's
patch as is.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/4/17 11:49 AM, Peter Eisentraut wrote:

I wonder whether we should emphasize this change by assigning
DO_REFRESH_MATVIEW a higher number, like 100?

Since there wasn't any interest in that idea, I have committed Jim's
patch as is.

Thanks. Something else that seems somewhat useful would be to have the
sort defined by an array of the ENUM values in the correct order, and
then have the code do the mechanical map generation. I'm guessing the
only reasonable way to make that work would be to have some kind of a
last item indicator value, so you know how many values were in the ENUM.
Maybe there's a better way to do that...
--
Jim Nasby, Chief Data Architect, OpenSCG

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On Sat, Mar 04, 2017 at 02:49:36PM -0500, Peter Eisentraut wrote:

On 3/1/17 08:36, Peter Eisentraut wrote:

On 2/22/17 18:24, Jim Nasby wrote:

Yes, by that logic matview refresh should always be last.

Patches for head attached.

RLS was the first item added after DO_REFRESH_MATVIEW, which was added
in 9.5. So if we want to treat this as a bug, they'd need to be patched
as well, which is a simple matter of swapping 33 and 34.

I wonder whether we should emphasize this change by assigning
DO_REFRESH_MATVIEW a higher number, like 100?

Since there wasn't any interest in that idea, I have committed Jim's
patch as is.

Would this be a candidate for backpatching, or is the behaviour change
in pg_dump trumping the issues it solves?

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB M�nchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 M�nchengladbach
Gesch�ftsf�hrung: Dr. Michael Meskes, J�rg Folz, Sascha Heuer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/6/17 03:33, Michael Banck wrote:

Would this be a candidate for backpatching, or is the behaviour change
in pg_dump trumping the issues it solves?

Unless someone literally has a materialized view on pg_policy, it
wouldn't make a difference, so I'm not very keen on bothering to
backpatch this.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 3/6/17 03:33, Michael Banck wrote:

Would this be a candidate for backpatching, or is the behaviour change
in pg_dump trumping the issues it solves?

Unless someone literally has a materialized view on pg_policy, it
wouldn't make a difference, so I'm not very keen on bothering to
backpatch this.

Agreed.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 3/6/17 03:33, Michael Banck wrote:

Would this be a candidate for backpatching, or is the behaviour change
in pg_dump trumping the issues it solves?

Unless someone literally has a materialized view on pg_policy, it
wouldn't make a difference, so I'm not very keen on bothering to
backpatch this.

Agreed.

So actually, the problem with Jim's patch is that it doesn't fix the
problem. pg_dump's attempts to REFRESH matviews will still fail in
common cases, because they still come out before GRANTs, because pg_dump
treats ACLs as a completely independent thing to be done last. This
was noted as far back as 2015 (in a thread previously linked from this
thread), and it's also the cause of Jordan Gigov's current complaint at
/messages/by-id/CA+nBocAmQ+OPNSKUzaaLa-6eGYVw5KqexWJaRoGvrvLyDir9gg@mail.gmail.com

Digging around in the archives, I find that Kevin had already proposed
a fix in
/messages/by-id/20160202161407.2778.24659@wrigleys.postgresql.org
which I didn't particularly care for, and apparently nobody else did
either. But we really oughta do *something*.

The main problem with Kevin's fix, after thinking about it more, is that
it shoves matview refresh commands into the same final processing phase
where ACLs are done, which means that in a parallel restore they will not
be done in parallel. That seems like a pretty serious objection, although
maybe not so serious that we'd be willing to accept a major rewrite in the
back branches to avoid it.

I'm wondering at this point about having restore create a fake DO_ACLS
object (fake in the sense that it isn't in the dump file) that would
participate normally in the dependency sort, and which we'd give a
priority before matview refreshes but after everything else. "Restore"
of that object would perform the same operation we do now of running
through the whole TOC and emitting grants/revokes. So it couldn't be
parallelized in itself (at least not without an additional batch of work)
but it could be treated as an indivisible parallelized task, and then the
matview refreshes could be parallelizable tasks after that.

There's also Peter's proposal of splitting up GRANTs from REVOKEs and
putting only the latter at the end. I'm not quite convinced that that's
a good idea but it certainly merits consideration.

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:

The main problem with Kevin's fix, after thinking about it more, is that
it shoves matview refresh commands into the same final processing phase
where ACLs are done, which means that in a parallel restore they will not
be done in parallel. That seems like a pretty serious objection, although
maybe not so serious that we'd be willing to accept a major rewrite in the
back branches to avoid it.

I'm wondering at this point about having restore create a fake DO_ACLS
object (fake in the sense that it isn't in the dump file) that would
participate normally in the dependency sort, and which we'd give a
priority before matview refreshes but after everything else. "Restore"
of that object would perform the same operation we do now of running
through the whole TOC and emitting grants/revokes. So it couldn't be
parallelized in itself (at least not without an additional batch of work)
but it could be treated as an indivisible parallelized task, and then the
matview refreshes could be parallelizable tasks after that.

There's also Peter's proposal of splitting up GRANTs from REVOKEs and
putting only the latter at the end. I'm not quite convinced that that's
a good idea but it certainly merits consideration.

After studying things for awhile, I've concluded that that last option
is probably not workable. ACL items contain a blob of SQL that would be
tricky to pull apart, and is both version and options dependent, and
contains ordering dependencies that seem likely to defeat any desire
to put the REVOKEs last anyway.

Instead, I've prepared the attached draft patch, which addresses the
problem by teaching pg_backup_archiver.c to process TOC entries in
three separate passes, "main" then ACLs then matview refreshes.
It's survived light testing but could doubtless use further review.

Another way we could attack this is to adopt something similar to
the PRE_DATA_BOUNDARY/POST_DATA_BOUNDARY mechanism; that is, invent more
dummy section boundary objects, add dependencies sufficient to constrain
all TOC objects to be before or after the appropriate boundaries, and
then let the dependency sort go at it. But I think that way is probably
more expensive than this one, and it doesn't have any real advantage if
there's not a potential for circular dependencies that need to be broken.
If somebody else wants to try drafting a patch like that, I won't stand
in the way, but I don't wanna do so.

Not clear where we want to go from here. Should we try to get this
into next month's minor releases, or review it in September's commitfest
and back-patch after that?

regards, tom lane

On Tue, Jul 25, 2017 at 10:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Instead, I've prepared the attached draft patch, which addresses the
problem by teaching pg_backup_archiver.c to process TOC entries in
three separate passes, "main" then ACLs then matview refreshes.
It's survived light testing but could doubtless use further review.

What worries me a bit is the possibility that something might depend
on a matview having already been refreshed. I cannot immediately
think of a case whether such a thing happens that you won't dismiss as
wrong-headed, but how sure are we that none such will arise?

I mean, a case that would actually break is if you had a CHECK
constraint or a functional index that contained a function which
referenced a materialized view for some validation or transformation
purpose. Then it wouldn't be formally immutable, of course. But
maybe we can imagine that some other case not involving lying could
exist, or come to exist.

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