Re: GIN logging GIN_SEGMENT_UNMODIFIED actions?

On Wed, Aug 31, 2016 at 12:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Fujii Masao <masao.fujii@gmail.com> writes:
>> I found that pg_xlogdump code for XLOG_GIN_INSERT record with
>> GIN_INSERT_ISLEAF flag has the same issue, i.e.,
>> "unknown action 0" error is thrown for that record.
>> The latest patch fixes this.
>
> Hmm, comparing gin_desc() to ginRedoInsert() makes me think there are more
> problems there than that one.  Aren't the references to "payload" wrong
> in all three branches of that "if" construct, not just the middle one?
> I'm inclined to suspect we should restructure this more like
>
>             {
>                 ginxlogInsert *xlrec = (ginxlogInsert *) rec;
> -               char       *payload = rec + sizeof(ginxlogInsert);
>
>                 appendStringInfo(buf, "isdata: %c isleaf: %c",
>                               (xlrec->flags & GIN_INSERT_ISDATA) ? 'T' : 'F',
>                              (xlrec->flags & GIN_INSERT_ISLEAF) ? 'T' : 'F');
>                 if (!(xlrec->flags & GIN_INSERT_ISLEAF))
>                 {
> +                   char       *payload = rec + sizeof(ginxlogInsert);
>                     BlockNumber leftChildBlkno;
>                     BlockNumber rightChildBlkno;
>
>                     leftChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
>                     payload += sizeof(BlockIdData);
>                     rightChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
>                     payload += sizeof(BlockNumber);
>                     appendStringInfo(buf, " children: %u/%u",
>                                      leftChildBlkno, rightChildBlkno);
>                 }
> +               if (XLogRecHasBlockImage(record, 0))
> +                   appendStringInfoString(buf, " (full page image)");
> +               else
> +               {
> +                   char       *payload = XLogRecGetBlockData(record, 0, NULL);
> +
>                     if (!(xlrec->flags & GIN_INSERT_ISDATA))
>                         appendStringInfo(buf, " isdelete: %c",
>                         (((ginxlogInsertEntry *) payload)->isDelete) ? 'T' : 'F');
>                     ... etc etc ...

If we do this, the extra information like ginxlogInsertEntry->isDelete will
never be reported when the record has FPW. This is OK in terms of REDO
because REDO logic just restores FPW and doesn't see isDelete in that case.
However if gin_desc() was initially implemented to dump any information
contained in WAL record as much as possible even when it had FPW,
we should not do such restructure. Not sure the initial intention for that.

For the purpose of debugging WAL generation code, I think that it's better
to dump even information that REDO logic doesn't use.

BTW, whether the record has FPW or not is reported in other places like
XLogDumpDisplayRecord() and xlog_outrec(). So we can check whether
FPW is contained or not, from the output of pg_xlogdump, even if
gin_desc doesn't report "(full page image)".

Regards,

-- 
Fujii Masao