Re: Logical Replication WIP

On 11/4/16 9:00 AM, Andres Freund wrote:
> + <para>
> + The <structname>pg_publication_rel</structname> catalog contains
> + mapping between tables and publications in the database. This is many to
> + many mapping.
> + </para>
>
> I wonder if we shouldn't abstract this a bit away from relations to
> allow other objects to be exported to. Could structure it a bit more
> like pg_depend.

I think we can add/change that when we have use for it.

> + <varlistentry>
> + <term><literal>FOR TABLE ALL IN SCHEMA</literal></term>
> + <listitem>
> + <para>
> + Specifies optional schema for which all logged tables will be added to
> + publication.
> + </para>
> + </listitem>
> + </varlistentry>
>
> "FOR TABLE ALL IN SCHEMA" sounds weird.

That clause no longer exists anyway.

> + <para>
> + This operation does not reserve any resources on the server. It only
> + defines grouping and filtering logic for future subscribers.
> + </para>
>
> That's strictly speaking not true, maybe rephrase a bit?

Maybe the point is that it does not initiate any contact with remote nodes.

> +/*
> + * Create new publication.
> + * TODO ACL check
> + */
>
> Hm?

The first patch is going to be just superuser and replication role. I'm
working on a patch set for later that adds proper ACLs, owners, and all
that. So I'd suggest to ignore these details for now, unless of course
you find permission checks *missing*.

> +/*
> + * Drop publication by OID
> + */
> +void
> +DropPublicationById(Oid pubid)
> +
> +/*
> + * Remove relation from publication by mapping OID.
> + */
> +void
> +RemovePublicationRelById(Oid proid)
> +{
>
> Permission checks?
>
> +}
>
> Hm. Neither of these does dependency checking, wonder if that can be
> argued to be problematic.

The dependency checking is done before it gets to these functions, no?

> /*
> + * Check if command can be executed with current replica identity.
> + */
> +static void
> +CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
> +{
> + PublicationActions *pubactions;
> +
> + /* We only need to do checks for UPDATE and DELETE. */
> + if (cmd != CMD_UPDATE && cmd != CMD_DELETE)
> + return;
> +
> + /* If relation has replica identity we are always good. */
> + if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
> + OidIsValid(RelationGetReplicaIndex(rel)))
> + return;
> +
> + /*
> + * This is either UPDATE OR DELETE and there is no replica identity.
> + *
> + * Check if the table publishes UPDATES or DELETES.
> + */
> + pubactions = GetRelationPublicationActions(rel);
> + if (pubactions->pubupdate || pubactions->pubdelete)
>
> I think that leads to spurious errors. Consider a DELETE with a
> publication that replicates updates but not deletes.

Yeah, it needs to check the pubactions against the specific command.

> +} FormData_pg_publication;
>
> Shouldn't this have an owner?

Yes, see above.

> I also wonder if we want an easier to
> extend form of pubinsert/update/delete (say to add pubddl, pubtruncate,
> pub ... without changing the schema).

Maybe, but how? (without using weird array constructs that are a pain
to parse in psql and pg_dump, for example)

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services