Re: adding partitioned tables to publications

On Wed, Mar 18, 2020 at 12:06 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Hi Peter,
>
> On Mon, Mar 16, 2020 at 9:49 PM Peter Eisentraut
> <peter.eisentraut@2ndquadrant.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