pg_get_object_address reports a published relation as non-existent - Mailing list pgsql-hackers

Hi,

In the "Distinguish publication exclusions in object addresses" thread,
Peter Smith ran into the case below, Amit suggested starting a new
thread for it [1], and shveta noted that it happens with TABLES IN
SCHEMA too and that no documentation describes it [2].  Here it is, with
a patch.

The case
--------

    CREATE TABLE t1(a int);
    CREATE PUBLICATION pub FOR ALL TABLES;

    SELECT schemaname, tablename FROM pg_publication_tables
     WHERE pubname = 'pub';
     schemaname | tablename
    ------------+-----------
     public     | t1

    SELECT pg_get_object_address('publication relation','{public,t1}','{pub}');
    ERROR:  publication relation "t1" in publication "pub" does not exist

One catalog view says the relation is published by that publication;
the other interface says it does not exist.  Both are right in their own
terms -- a FOR ALL TABLES publication stores no pg_publication_rel entry,
so there is no object of that kind to address -- but the message does
not say that, and a user comparing the two has nothing to go on.  FOR
TABLES IN SCHEMA behaves the same way, and so does a partition published
through its partitioned ancestor.

It still reproduces on REL_19_STABLE after 91ff666f1d81, and 94670ba6d56
on master leaves this error unchanged.  It reproduced identically on 13
through 18.

The patch
---------

I did not change what the function returns: the address really does
not exist, and inventing one would be worse.  The patch adds a detail
that says why:

    ERROR:  publication relation "t1" in publication "pub" does not exist
    DETAIL:  Table "t1" is published by publication "pub" without an entry
             of its own, through FOR ALL TABLES, FOR TABLES IN SCHEMA, or
             a partitioned ancestor.

The detail is emitted only when the table really is published; a
relation that is not still gets the plain message.  To decide that, the
patch uses is_table_publishable_in_publication(), the test
pg_get_publication_tables() already applies when filtering by relation,
and exports it from pg_publication.c.  It only touches the "publication
relation" branch, not the one for excluded relations.

To check that choice, the attached pubrel_detail_matrix.sql tries 11
relations (plain, unlogged, view, sequence, a table in a schema, the
partitioned tables and their partitions, and tables in an EXCEPT
clause) against 6 publications (FOR ALL TABLES with and without
publish_via_partition_root, with EXCEPT, TABLES IN SCHEMA, and FOR TABLE
on a partitioned table with and without publish_via_partition_root),
and compares pg_publication_tables with what pg_get_object_address()
reports.  Of the 66 pairs, 62 reach this error:

  * REL_19_STABLE without the patch: 18 of those 62 are published
    according to pg_publication_tables, and all 18 get the plain
    message.

  * master with the patch: the detail appears on exactly those 18 and
    on none of the other 44.

  * A cheaper test, looking only at the publication kind (FOR ALL TABLES,
    or the relation's schema in the publication), which would have
    avoided exporting anything, is wrong in 18 of the 62: it would add
    the detail for views, sequences, unlogged tables, partitions of a
    table in the EXCEPT clause, and whichever of a partitioned table or
    its partitions publish_via_partition_root leaves out, and it would
    miss partitions published through their ancestor.

The remaining 4 pairs are the two tables that do have an entry and the
two EXCEPT tables, which now get the message added by 94670ba6d56.

The patch adds tests to object_address for the schema case, the
partition-through-ancestor case, and a relation that is not published.
They use FOR TABLES IN SCHEMA and FOR TABLE rather than FOR ALL TABLES,
because a FOR ALL TABLES publication in the regression database disturbs
the tests running in parallel with it.  It applies to master at
bca67e5a33b, builds without warnings, and make check passes.

I did not touch the docs.  The pg_get_object_address() entry does not
describe any object type on its own, so a sentence about this one might
not belong there; if you think it does, or know a better place, I will
add it.

Two things I am not sure about, and would rather ask than guess:

  1. Is a detail the right weight for this, or would you rather the
     message itself were reworded?

  2. Is exporting is_table_publishable_in_publication() for a message
     acceptable, or would you rather keep it static and have the message
     be less precise?

[1] https://postgr.es/m/CAA4eK1++NMJbD4aiahiH6N8kx01-MEcC87KPMZ_BBvK_qTdmqA@mail.gmail.com
[2] https://postgr.es/m/CAJpy0uCp+43rgbPE9v8Zy0FXq=nssizOsbaF0NNaezTK65XVYQ@mail.gmail.com

Regards,
Manu