Re: Restrict publishing of partitioned table with a foreign table as partition - Mailing list pgsql-hackers

As promised, here's a rundown of the changes I did, mostly in order the
patch shows them:

- I reworded the documentation changes to read more coherent with the
surrounding text.

- It seemed wrong to have check_publication_add_relation() have the
relation first as argument and publication later, so I turned them
around.

- I rewrote almost all of the error messages.  The errdetails failed
style (no initial capitals and no end period); one of the errmsg() was
completely wrong (it said "cannot set parameter" instead of "cannot add
table to publication").  I also moved the parameter name out of the
translatable part of the message.

- I removed the novel use of the term "foreign partition".  We don't
seem to use that anywhere that's user-visible, only in code comments.
Instead I used "a partition that's a foreign table".  I just did a new
grep and I think I missed a couple, which I'm going to fix momentarily.

- I renamed check_foreign_tables to publication_check_foreign_parts and
moved to a different place in the file.  The original name wasn't very
good for an extern function.

- I renamed check_partrel_has_foreign_table to RelationHasForeignPartition 
and moved it to partdesc.c.  There's nothing that ties this function to
pg_publication.c, so in my mind it made no sense for it to be there.
I'm not 100% certain that partdesc.c is the best place for it, but if
there's another, then I didn't see it.  Also, I made it Assert() that
it's not given a relation other than a partitioned table, which forces
all callers to check the relkind before calling it.  This is good
because we don't need to open tables before ensuring the relkind is one
that interests us.  Also, I made it use the PartitionDesc->is_leaf flag
array to determine whether to recurse or not; and use get_rel_relkind to
get the relkind for leaf partitions, which is much cheaper than doing
table_open.  The original coding was kind of wasteful for no reason.

- The check to prohibit CREATE FOREIGN TABLE as a partition in
CreateForeignTable, was entirely in the wrong place.  This is proven by
the fact that the original coding had to do table_openrv() in order to
know what the parent table was.  This is terrible coding, because it
means we resolve the table name to OID twice.  (This is probably not so
terrible because the first location that does it already acquires a good
enough lock that it'd stay put; still, it's a bad coding pattern).  I
moved it to DefineRelation, in the block were we learn that we're
creating a partition; it's easy to check there that the new relation is
a foreign table.  We don't even need to open the parent table at all,
since it's already open.

- In ATExecAttachPartition (and in the check that was in
CreateForeignTable), there were a bunch of list_concat_unique_oids().
It's unlikely that this would bother anyone unless there are large
numbers of publications, but the performance characteristics of doing
this uniquification were unclear, and list_concat_unique() comments warn
against it anyway.  It seemed safer to just do list_concat() and a
single sort/uniq step at the end, just before walking the list.

- There were a bunch of list_frees() in there.  I removed them all.
They are pointless, unnecessary, and give a wrong sense of cleanliness.
In reality, you will be leaking the second argument of each
list_concat() anyway; to make this watertight, you'd have to read each
of those lists into a variable, concat, then free the variable.  The
code would become much noisier coding.  However, DDL code is normally
leaky and that causes no major problems, because the memory context in
use is going to be released or deleted soon afterwards.  Freeing a few
bytes ahead of time would only require more memory context bookkeeping
to be executed, to very little gain.


I think that's all I had ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/