Re: Logical Replication WIP

On 02/09/16 22:57, Peter Eisentraut wrote:
> Review of 0001-Add-PUBLICATION-catalogs-and-DDL.patch:
>

Thanks!

> 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?
>

Seems reasonable.

> 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