Re: adding partitioned tables to publications

On Wed, Mar 18, 2020 at 12:06 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> Hi Peter,
>
> On Mon, Mar 16, 2020 at 9:49 PM Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> >
> > I was trying to extract some preparatory work from the remaining patches
> > and came up with the attached. This is part of your patch 0003, but
> > also relevant for part 0004. The problem was that COPY (SELECT *) is
> > not sufficient when the table has generated columns, so we need to build
> > the column list explicitly.
> >
> > Thoughts?
>
> Thank you for that.
>
> + if (isnull || !remote_is_publishable)
> + ereport(ERROR,
> + (errmsg("table \"%s.%s\" on the publisher is not publishable",
> + nspname, relname)));
>
> Maybe add a one-line comment above this to say it's an "not supposed
> to happen" error or am I missing something? Wouldn't elog() suffice
> for this?
>
> Other than that, looks good.

Wait, the following Assert in copy_table() should now be gone:

Assert(relmapentry->localrel->rd_rel->relkind == RELKIND_RELATION);

because just below it:

/* Start copy on the publisher. */
initStringInfo(&cmd);
- appendStringInfo(&cmd, "COPY %s TO STDOUT",
- quote_qualified_identifier(lrel.nspname, lrel.relname));
+ if (lrel.relkind == RELKIND_RELATION)
+ appendStringInfo(&cmd, "COPY %s TO STDOUT",
+ quote_qualified_identifier(lrel.nspname,
lrel.relname));
+ else
+ {
+ /*
+ * For non-tables, we need to do COPY (SELECT ...), but we can't just
+ * do SELECT * because we need to not copy generated columns.
+ */

By the way, I have rebased the patches, although maybe you've got your
own copies; attached.

--
Thank you,
Amit