Review of 0001-Add-PUBLICATION-catalogs-and-DDL.patch:
The new system catalog pg_publication_rel has columns pubid, relid,
and does not use the customary column name prefixes. Maybe that is OK
here. I can't actually think of a naming scheme that wouldn't make
things worse.
The hunk in patch 0006 for
src/backend/replication/logical/publication.c needs to be moved to
0001, for the definition of GetPublicationRelations().
The catalog column puballtables is not mentioned in the documentation.
Unrelated formatting changes in src/backend/commands/Makefile.
In psql, the code psql_error("The server (version %d.%d) does not
...") should be updated to use the new formatPGVersionNumber()
function.
In psql, psql \dr is already for "roles" (\drds). You are adding \drp
for publications. Maybe use big R for replication-related describes?
There should be some documentation about how TRUNCATE commands are
handled by publications. Patch 0005 mentions TRUNCATE in the general
documentation, but I would have questions when reading the CREATE
PUBLICATION reference page.
Also, document how publications deal with INSERT ON CONFLICT.
In some places, the new publication object type is just added to the
end of a list instead of some alphabetical place, e.g.,
event_trigger.c, gram.y (drop_type).
publication.h has /* true if inserts are replicated */ repeated
several times.
What are the BKI_ROWTYPE_OID assignments for? Are they necessary
here? (Maybe this was just copied from pg_subscription?)
I think some or all of replication/logical/publication.c should be
catalog/pg_publication.c. There are various different precedents in
how this can be split up, but I kind of like having command/foocmds.c
call into catalog/pg_foo.c.
Also, some things could be in lsyscache.c, although not too many new
things go in there now.
Most calls of the GetPublication() function could be changed to a
simpler get_publication_name(Oid), because that is all it is used for
so far. (It will be used later in 0003, but only in one specific
case.)
In get_object_address_publication_rel() you are calling
ObjectAddressSet(address, UserMappingRelationId, InvalidOid). That is
probably a typo.
Also, document somewhere around get_object_address_publication_rel()
what objname (relation) and objargs (publication) are, otherwise one
has to guess. (Existing similar functions are also not good about that.)
The code for OCLASS_PUBLICATION_REL in getObjectIdentityParts() does
not fill in objname and objargs, as it is supposed to.
If I add a table to a publication, it requires a primary key. But
after the table is added, I can remove the primary key. There is code
in publication_add_relation() to record dependencies for that, but it
doesn't seem to do its job right.
Relatedly, the error messages in check_publication_add_relation() and
AlterPublicationOptions() conflate replica identity index and primary
key. (I suppose the whole user-facing presentation of what replica
identity indexes are, which have so far been a rather obscure feature,
will need some polishing during this.)
I think the syntax could be made prettier. For example, instead of
CREATE PUBLICATION testpib_ins_trunct WITH noreplicate_delete
noreplicate_update;
how about something like
CREATE PUBLICATION foo (REPLICATE DELETE, NO REPLICATE UPDATE);
Not that important right now, but something to keep in mind.
I also found ALTER PUBLICATION FOR TABLE / FOR ALL TABLES confusing.
Maybe that should be SET TABLE or something.
Finally, I'd like some more test coverage of DDL error cases, like
adding a view to a publication, trying to drop a primary key (as per
above), and so on.
(Various small typos and such I didn't bother with at this time.)
--
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
