Re: Logical Replication - detail message with names of missing columns

Thanks Amit for the review comments.

On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Few comments:
> ==============
> 1.
> + /* Report error with names of the missing localrel column(s). */
> + if (!bms_is_empty(missingatts))
> + {
> + StringInfoData missingattsbuf;
> + int missingattcnt = 0;
> + remoterel->nspname,
> + remoterel->relname,
> + missingattsbuf.data)));
> + }
>
> I think it is better to move the above code in a separate function
> (say logicalrep_report_missing_attrs or something like that).
>

Added a new function logicalrep_report_missing_attrs().

>
> 2. I think we always need to call bms_free(missingatts) because it is
> possible that there is no missing attribute and in that case, we won't
> free the memory allocated in bms_add_range.
>

Done. Yes we palloc memory for missingatts bitmap irrespective of missing
attributes. Added bms_free() out of if(!bms_is_empty(missingatts)) as well.
I also kept bms_free() before ereport(ERROR,..) to free up before throwing
the error. In anycase, only one bms_free() would get hit.

if (!bms_is_empty(missingatts))
{
StringInfoData missingattsbuf;
int missingattcnt = 0;

bms_free(missingatts);
ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg_plural("logical replication target relation \"%s.%s\" is missing
replicated column: %s",
"logical replication target relation \"%s.%s\" is missing replicated
columns: %s",
missingattcnt,
remoterel->nspname,
remoterel->relname,
missingattsbuf.data)));
}
bms_free(missingatts);

>
> 3. The patch doesn't seem to be freeing the memory allocated for
missingattsbuf.
>

I don't think we need to do that. We are passing missingattsbuf.data to
ereport and we are safe without freeing up missingattsbuf(we don't reach
the code after ereprot(ERROR,...)as the table sync worker anyways goes away
after throwing missing attributes error( if (sigsetjmp(local_sigjmp_buf, 1)
!= 0) in StartBackgroundWorker and then proc_exit(1)). Each time a new
table sync bg worker is spawned.

2020-10-06 10:18:27.063 IST [1599963] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"
2020-10-06 10:18:47.179 IST [1600134] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"
2020-10-06 10:18:57.234 IST [1600214] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"

2020-10-06 10:19:27.415 IST [1600458] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"
2020-10-06 10:19:42.506 IST [1600588] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"
2020-10-06 10:19:52.565 IST [1600669] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"

>
> 4.
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> - errmsg("logical replication target relation \"%s.%s\" is missing "
>
> From the second line onwards, the message lines are not aligned in
> errmsg_plural.
>

Done.

>
> 5. Also, in the above message, keep the error string in a single line.
> For ex. see one of the existing messages:
> errmsg_plural("WAL segment size must be a power of two between 1 MB
> and 1 GB, but the control file specifies %d byte", .. I think it will
> be easy to read that way. I know this is not exactly related to your
> patch but improving it while changing this message seems fine.
>

Done.

Attaching v7 patch please consider it for further review.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com