Re: BUG #18203: Logical Replication initial sync failure

On Mon, Nov 20, 2023 at 8:23 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
...
> Thanks for reporting this issue, I was able to reproduce the issue.
> This issue is happening because we are trying to specify the column
> list while the table sync worker is copying data for the table.
> I felt we should not specify the column list when the table has no columns.
> Attached patch has the changes to handle the same.
>

Hi Vignesh,

One small comment about the patch:

- appendStringInfo(&cmd, "COPY %s (",
+ appendStringInfo(&cmd, "COPY %s ",
quote_qualified_identifier(lrel.nspname, lrel.relname));

- /*
- * XXX Do we need to list the columns in all cases? Maybe we're
- * replicating all columns?
- */
- for (int i = 0; i < lrel.natts; i++)
+ /* If the table has columns, then specify the columns */
+ if (lrel.natts)
{
- if (i > 0)
- appendStringInfoString(&cmd, ", ");
+ appendStringInfoString(&cmd, "(");

For consistent whitespace handling, and to prevent double-spacing when
there are no columns, I think that the trailing space of "COPY %s "
should be removed/replaced by a leading space for the "(".

======
Kind Regards,
Peter Smith.
Fujitsu Australia