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 (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers