On 04/11/16 14:00, Andres Freund wrote:
> Hi,
>
> + <sect1 id="catalog-pg-publication-rel">
> + <title><structname>pg_publication_rel</structname></title>
> +
> + <indexterm zone="catalog-pg-publication-rel">
> + <primary>pg_publication_rel</primary>
> + </indexterm>
> +
> + <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.
>
Honestly, let's not overdesign this. Change like that can be made in the
future if we need it and I am quite unconvinced we do given that
anything we might want to replicate will be relation. I understand that
it might be useful to know what's on downstream in terms of objects at
some point for some future functionality, but I am don't have idea how
that functionality will look like so it's premature to guess what
catalog structure it will need.
>
> +ALTER PUBLICATION <replaceable class="PARAMETER">name</replaceable> [ [ WITH
> ] <replaceable class="PARAMETER">option</replaceable> [ ... ] ]
> +
> +<phrase>where <replaceable class="PARAMETER">option</replaceable> can
> be:</phrase>
> +
> + PuBLISH_INSERT | NOPuBLISH_INSERT
> + | PuBLISH_UPDATE | NOPuBLISH_UPDATE
> + | PuBLISH_DELETE | NOPuBLISH_DELETE
>
> That's odd casing.
>
>
> + <varlistentry>
> + <term><literal>PuBLISH_INSERT</literal></term>
> + <term><literal>NOPuBLISH_INSERT</literal></term>
> + <term><literal>PuBLISH_UPDATE</literal></term>
> + <term><literal>NOPuBLISH_UPDATE</literal></term>
> + <term><literal>PuBLISH_DELETE</literal></term>
> + <term><literal>NOPuBLISH_DELETE</literal></term>
>
Ah typo in my sed script, fun.
> More odd casing.
>
> + <varlistentry>
> + <term><literal>FOR TABLE</literal></term>
> + <listitem>
> + <para>
> + Specifies optional list of tables to add to the publication.
> + </para>
> + </listitem>
> + </varlistentry>
> +
> + <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.
>
I actually removed support for this at some point, forgot to remove
docs. I might add this feature again in the future but I reckon we can
live without it in v1.
> + <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?
>
Sure, this basically is supposed to mean that it does not really start
replication or keep wal or anything like that as opposed what for
example slots do.
> +/*
> + * Check if relation can be in given publication and throws appropriate
> + * error if not.
> + */
> +static void
> +check_publication_add_relation(Relation targetrel)
> +{
> + /* Must be table */
> + if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("only tables can be added to
> publication"),
> + errdetail("%s is not a table",
> +
> RelationGetRelationName(targetrel))));
> +
> + /* Can't be system table */
> + if (IsCatalogRelation(targetrel))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("only user tables can be added to
> publication"),
> + errdetail("%s is a system table",
> +
> RelationGetRelationName(targetrel))));
> +
> + /* UNLOGGED and TEMP relations cannot be part of publication. */
> + if (!RelationNeedsWAL(targetrel))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("UNLOGGED and TEMP relations cannot be
> replicated")));
> +}
>
> This probably means we need a check in the ALTER TABLE ... SET UNLOGGED
> path.
>
Good point.
>
> +/*
> + * Returns if relation represented by oid and Form_pg_class entry
> + * is publishable.
> + *
> + * Does same checks as the above, but does not need relation to be opened
> + * and also does not throw errors.
> + */
> +static bool
> +is_publishable_class(Oid relid, Form_pg_class reltuple)
> +{
> + return reltuple->relkind == RELKIND_RELATION &&
> + !IsCatalogClass(relid, reltuple) &&
> + reltuple->relpersistence == RELPERSISTENCE_PERMANENT &&
> + /* XXX needed to exclude information_schema tables */
> + relid >= FirstNormalObjectId;
> +}
>
> Shouldn't that be IsCatalogRelation() instead?
>
Well IsCatalogRelation just calls IsCatalogClass and we call
IsCatalogClass here as well. The problem with IsCatalogClass is that it
does not consider tables in information_schema that were created as part
of initdb to be system catalogs because it first does negative check on
pg_catalog and toast schemas and only then considers
FirstNormalObjectId. I was actually wondering if that might be a bug in
IsCatalogClass.
>
> +/*
> + * Create new publication.
> + * TODO ACL check
> + */
>
That was meant for future enhancements, but I think I'll don't do
detailed ACLs in v1 so I'll remove that TODO.
> +
> +/*
> + * 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.
>
As PeterE said, that's done by caller, none of the Drop...ById does
dependency checks.
> +publication_opt_item:
> + IDENT
> + {
> + /*
> + * We handle identifiers that aren't
> parser keywords with
> + * the following special-case codes, to
> avoid bloating the
> + * size of the main parser.
> + */
> + if (strcmp($1, "publish_insert") == 0)
> + $$ =
> makeDefElem("publish_insert",
> +
> (Node *)makeInteger(TRUE), @1);
> + else if (strcmp($1, "nopublish_insert")
> == 0)
> + $$ =
> makeDefElem("publish_insert",
> +
> (Node *)makeInteger(FALSE), @1);
> + else if (strcmp($1, "publish_update")
> == 0)
> + $$ =
> makeDefElem("publish_update",
> +
> (Node *)makeInteger(TRUE), @1);
> + else if (strcmp($1, "nopublish_update")
> == 0)
> + $$ =
> makeDefElem("publish_update",
> +
> (Node *)makeInteger(FALSE), @1);
> + else if (strcmp($1, "publish_delete")
> == 0)
> + $$ =
> makeDefElem("publish_delete",
> +
> (Node *)makeInteger(TRUE), @1);
> + else if (strcmp($1, "nopublish_delete")
> == 0)
> + $$ =
> makeDefElem("publish_delete",
> +
> (Node *)makeInteger(FALSE), @1);
> + else
> + ereport(ERROR,
> +
> (errcode(ERRCODE_SYNTAX_ERROR),
> +
> errmsg("unrecognized publication option \"%s\"", $1),
> +
> parser_errposition(@1)));
> + }
> + ;
>
> I still would very much like to move this outside of gram.y and just use
> IDENTs here. Like how COPY options are handled.
>
Well, I looked into it and it means some loss of info in the error
messages - mainly the error position in the query because utility
statements don't get ParseState (unlike COPY). It might be worth the
flexibility though.
>
>
> +CATALOG(pg_publication,6104)
> +{
> + NameData pubname; /* name of the
> publication */
> +
> + /*
> + * indicates that this is special publication which should encompass
> + * all tables in the database (except for the unlogged and temp ones)
> + */
> + bool puballtables;
> +
> + /* true if inserts are published */
> + bool pubinsert;
> +
> + /* true if updates are published */
> + bool pubupdate;
> +
> + /* true if deletes are published */
> + bool pubdelete;
> +
> +} FormData_pg_publication;
>
> Shouldn't this have an owner?
Probably, I wanted to do that as follow-up patch originally, but looks
like it should be in initial version.
> 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).
>
So like, text array that's then parsed everywhere (I am not doing
bitmask/int definitely)?
>
> +/* ----------------
> + * pg_publication_rel definition. cpp turns this into
> + * typedef struct FormData_pg_publication_rel
> + *
> + * ----------------
> + */
> +#define PublicationRelRelationId 6106
> +
> +CATALOG(pg_publication_rel,6106)
> +{
> + Oid prpubid; /* Oid of the
> publication */
> + Oid prrelid; /* Oid of the
> relation */
> +} FormData_pg_publication_rel;
>
> To me it seems like a good idea to have objclassid/objsubid here.
>
You said that in the beginning, but again I am not quite convinced of
that yet. i guess if PeterE will move the sequence patches all the way
and we might lose the notion that sequences are relation (not sure if
that's where he is ultimately going though), that might make sense,
otherwise, don't really think this we need that.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
