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