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.kapila16@gmail.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.
>
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