On 02/09/16 22:57, Peter Eisentraut wrote:
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.
Yeah, well I could not either and thee are some catalogs that don't use the prefixes so I figured it's probably not big deal.
In psql, the code psql_error("The server (version %d.%d) does not ...") should be updated to use the new formatPGVersionNumber() function.
Right, same thing will be in the 2nd patch.
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.
That's actually bug in the 0005 patch, TRUNCATE is not handled ATM, but that should be probably documented as well.
Also, document how publications deal with INSERT ON CONFLICT.
Okay, they just replicate whatever was the result of that operation (if any).
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).
Hmm, what is and what isn't alphabetically sorted is quite unclear for me as we have mix of both everywhere. For example, if you consider drop_type to be alphabetically sorted then our locales are much more different than I thought :)
What are the BKI_ROWTYPE_OID assignments for? Are they necessary here? (Maybe this was just copied from pg_subscription?)
Yes they are.
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.
Okay, I prefer grouping the code by functionality (as in terms of this is replication) rather than architectures (as in terms this is catalog) but no problem moving it. Again same thing will be in 2nd patch.
Also, some things could be in lsyscache.c, although not too many new things go in there now.
TBH I dislike the whole lsyscache concept of just random lookup functions piled in one huge module and would rather not add to it.
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.)
You mean the calls from objectaddress? Will change that - I actually added the get_publication_name much later in the development and didn't go back to use it in preexisting code.
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.
I need to rewrite that part. That's actually something I could use other people opinion on - currently the pg_publication_rel does not have records for the alltables publication as that seemed redundant so it will need some special handling in tablecmds.c for the "dependency" tracking and possibly elsewhere for other things. I do wonder though if we should instead just add records to the pg_publication_rel catalog.
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.)
Those are copy/paste issues from pglogical. It should say replica identity index everywhere. But yes it might be needed to make it more obvious what replica identity indexes are.
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);
I went with the same syntax style as CREATE ROLE, but I am open to changes.
I also found ALTER PUBLICATION FOR TABLE / FOR ALL TABLES confusing. Maybe that should be SET TABLE or something.
Yeah I am not sure what is the best option there. SET was also what I was thinking but then it does not map well to the CREATE PUBLICATION syntax and I would like to have some harmony there.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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