Обсуждение: [PATCH] Sort policies and triggers by table name in pg_dump.

Hello all,

Currently pg_dump sorts most dumpable objects by priority, namespace, name
and then object ID. Since triggers and RLS policies belong to tables, there
may be more than one with the same name within the same namespace, leading to
potential sorting discrepancies between databases that only differ by object
IDs.

The attached draft patch (made against `pg_dump_sort.c` on master) breaks
ties for trigger and policy objects by using the table name, increasing the
sort order stability. I have compiled it and executed it against a number of
local databases and it behaves as desired.

I am new to PostgreSQL contribution and my C-skills are rusty, so please let
me know if I can improve the patch, or if there are areas of PostgreSQL that
I have overlooked.

Kind regards,

Benjie Gillam

On Mon, Sep 23, 2019 at 10:34:07PM +0100, Benjie Gillam wrote:
> The attached draft patch (made against `pg_dump_sort.c` on master) breaks
> ties for trigger and policy objects by using the table name, increasing the
> sort order stability. I have compiled it and executed it against a number of
> local databases and it behaves as desired.

Could you provide a simple example of schema (tables with some
policies and triggers), with the difference this generates for
pg_dump, which shows your point?

> I am new to PostgreSQL contribution and my C-skills are rusty, so please let
> me know if I can improve the patch, or if there are areas of PostgreSQL that
> I have overlooked.

Your patch has two warnings because you are trying to map a policy
info pointer to a trigger info pointer:
pg_dump_sort.c:224:24: warning: initialization of ‘TriggerInfo *’ {aka
‘struct _triggerInfo *’} from incompatible pointer type ‘PolicyInfo *
const’ {aka ‘struct _policyInfo * const’}
[-Wincompatible-pointer-types]
  224 |   TriggerInfo *tobj2 = *(PolicyInfo *const *) p2;
--
Michael

> Could you provide a simple example of schema (tables with some
> policies and triggers), with the difference this generates for
> pg_dump, which shows your point?

Certainly; I've attached a bash script that can reproduce the issue
and the diff that it produces, here's the important part:

    CREATE TRIGGER a BEFORE INSERT ON foo
        FOR EACH ROW EXECUTE PROCEDURE qux();
    CREATE POLICY a ON foo FOR SELECT USING (true);

    CREATE TRIGGER a BEFORE INSERT ON bar
        FOR EACH ROW EXECUTE PROCEDURE qux();
    CREATE POLICY a ON bar FOR SELECT USING (true);

Here we create two identically named triggers and two identically
named policies on tables foo and bar. If instead we ran these
statements in a different order (or if the object IDs were to wrap)
the order of the pg_dump would be different even though the
databases are identical other than object IDs. The attached
patch eliminates this difference.

> Your patch has two warnings because you are trying to map a policy
> info pointer to a trigger info pointer:

Ah, thank you for the pointer (aha); I've attached an updated patch
that addresses this copy/paste issue.

On Tue, Sep 24, 2019 at 08:48:33AM +0100, Benjie Gillam wrote:
> Here we create two identically named triggers and two identically
> named policies on tables foo and bar. If instead we ran these
> statements in a different order (or if the object IDs were to wrap)
> the order of the pg_dump would be different even though the
> databases are identical other than object IDs. The attached
> patch eliminates this difference.

Thanks.  Perhaps you could add your patch to the next commit fest
then at https://commitfest.postgresql.org/25/?

This way, your patch gains more visibility for potential reviews.
Another key thing to remember is that one patch authored requires one
other patch of equal difficulty to be reviewed.
--
Michael

> Thanks.  Perhaps you could add your patch to the next commit fest
> then at https://commitfest.postgresql.org/25/?

Thanks, submitted.

Benjie Gillam <benjie@jemjie.com> writes:
>> Your patch has two warnings because you are trying to map a policy
>> info pointer to a trigger info pointer:

> Ah, thank you for the pointer (aha); I've attached an updated patch
> that addresses this copy/paste issue.

LGTM, pushed (with a bit of extra polishing of comments).

            regards, tom lane