Re: Error with index on unlogged table

On 2015-11-27 16:59:20 +0900, Michael Paquier wrote:
> Attached is a patch that fixes the issue for me in master and 9.5.
> Actually in the last patch I forgot a call to smgrwrite to ensure that
> the INIT_FORKNUM is correctly synced to disk when those pages are
> replayed at recovery, letting the reset routines for unlogged
> relations do their job correctly. I have noticed as well that we need
> to do the same for gin and brin relations. In this case I think that
> we could limit the flush to unlogged relations, my patch does it
> unconditionally though to generalize the logic. Thoughts?

I think it's a good idea to limit this to unlogged relations. For a
dataset that fits into shared_buffers and creates many relations, the
additional disk writes could be noticeable.

> diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
> index 99337b0..d7964ac 100644
> --- a/src/backend/access/brin/brin.c
> +++ b/src/backend/access/brin/brin.c
> @@ -682,7 +682,12 @@ brinbuildempty(PG_FUNCTION_ARGS)
>      brin_metapage_init(BufferGetPage(metabuf), BrinGetPagesPerRange(index),
>                         BRIN_CURRENT_VERSION);
>      MarkBufferDirty(metabuf);
> -    log_newpage_buffer(metabuf, false);
> +    /*
> +     * When this full page image is replayed, there is no guarantee that
> +     * this page will be present to disk when replayed particularly for
> +     * unlogged relations, hence enforce it to be flushed to disk.
> +     */

The grammar seems a bit off here.

> +    /*
> +     * Initialize and xlog metabuffer and root buffer. When those full
> +     * pages are replayed, it is not guaranteed that those relation
> +     * init forks will be flushed to disk after replaying them, hence
> +     * enforce those pages to be flushed to disk at replay, only the
> +     * last record will enforce a flush for performance reasons and
> +     * because it is actually unnecessary to do it multiple times.
> +     */

That comment needs some love too. I think it really only makes sense if
you know the current state. There's also some language polishing needed.

> @@ -450,14 +450,21 @@ ginbuildempty(PG_FUNCTION_ARGS)
>      START_CRIT_SECTION();
>      GinInitMetabuffer(MetaBuffer);
>      MarkBufferDirty(MetaBuffer);
> -    log_newpage_buffer(MetaBuffer, false);
> +    log_newpage_buffer(MetaBuffer, false, false);
>      GinInitBuffer(RootBuffer, GIN_LEAF);
>      MarkBufferDirty(RootBuffer);
> -    log_newpage_buffer(RootBuffer, false);
> +    log_newpage_buffer(RootBuffer, false, true);
>      END_CRIT_SECTION();
>
Why isn't the metapage flushed to disk?

> --- a/src/backend/access/spgist/spginsert.c
> +++ b/src/backend/access/spgist/spginsert.c
> @@ -173,7 +173,7 @@ spgbuildempty(PG_FUNCTION_ARGS)
>                (char *) page, true);
>      if (XLogIsNeeded())
>          log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
> -                    SPGIST_METAPAGE_BLKNO, page, false);
> +                    SPGIST_METAPAGE_BLKNO, page, false, false);
>  
>      /* Likewise for the root page. */
>      SpGistInitPage(page, SPGIST_LEAF);
> @@ -183,7 +183,7 @@ spgbuildempty(PG_FUNCTION_ARGS)
>                (char *) page, true);
>      if (XLogIsNeeded())
>          log_newpage(&index->rd_smgr->smgr_rnode.node, INIT_FORKNUM,
> -                    SPGIST_ROOT_BLKNO, page, true);
> +                    SPGIST_ROOT_BLKNO, page, true, false);
>

I don't think it's correct to not flush in these cases. Yes, the primary
does an smgrimmedsync() - but that's not done on the standby.

> @@ -9392,9 +9396,28 @@ xlog_redo(XLogReaderState *record)
>           * when checksums are enabled. There is no difference in handling
>           * XLOG_FPI and XLOG_FPI_FOR_HINT records, they use a different info
>           * code just to distinguish them for statistics purposes.
> +         *
> +         * XLOG_FPI_FOR_SYNC records are generated when a relation needs to
> +         * be sync'ed just after replaying a full page. This is important
> +         * to match the master behavior in the case where a page is written
> +         * directly to disk without going through shared buffers, particularly
> +         * when writing init forks for index relations.
>           */

How about

FPI_FOR_SYNC records indicate that the page immediately needs to be
written to disk, not just to shared buffers. This is important if the
on-disk state is to be the authoritative, not the state in shared
buffers. E.g. because on-disk files may later be copied directly.

> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 0ddde72..eb22a51 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -9920,7 +9920,8 @@ copy_relation_data(SMgrRelation src, SMgrRelation dst,
>           * space.
>           */
>          if (use_wal)
> -            log_newpage(&dst->smgr_rnode.node, forkNum, blkno, page, false);
> +            log_newpage(&dst->smgr_rnode.node, forkNum, blkno, page,
> +                        false, false);

Shouldn't this flush data if it's an init fork? Currently that's an
academic distinction, given there'll never be a page, but it still seems
prudent.

>  extern void InitXLogInsert(void);
> diff --git a/src/include/catalog/pg_control.h b/src/include/catalog/pg_control.h
> index ad1eb4b..91445f1 100644
> --- a/src/include/catalog/pg_control.h
> +++ b/src/include/catalog/pg_control.h
> @@ -73,6 +73,7 @@ typedef struct CheckPoint
>  #define XLOG_END_OF_RECOVERY            0x90
>  #define XLOG_FPI_FOR_HINT                0xA0
>  #define XLOG_FPI                        0xB0
> +#define XLOG_FPI_FOR_SYNC                0xC0


I'm not a big fan of the XLOG_FPI_FOR_SYNC name. Syncing is a bit too
ambigous for my taste. How about either naming it XLOG_FPI_FLUSH or
instead adding actual record data and a 'flags' field in there? I
slightly prefer the latter - XLOG_FPI and XLOG_FPI_FOR_HINT really are
different, XLOG_FPI_FOR_SYNC not so much.

Greetings,

Andres Freund