public inbox for [email protected]
help / color / mirror / Atom feedFrom: Álvaro Herrera <[email protected]>
To: Shlok Kyal <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Sergey Tatarintsev <[email protected]>
Cc: [email protected]
Subject: Re: Restrict publishing of partitioned table with a foreign table as partition
Date: Mon, 7 Apr 2025 12:05:07 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
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/
view thread (3+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Restrict publishing of partitioned table with a foreign table as partition
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox