Re: Error with index on unlogged table

Hi,

On 2015-12-04 21:57:54 +0900, Michael Paquier wrote:
> On Fri, Dec 4, 2015 at 5:10 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> Let's go for XLOG_FPI_FLUSH.
> >
> > I think the other way is a bit better, because we can add new flags
> > without changing the WAL format.
>
> Hm. On the contrary, I think that it would make more sense to have a
> flag as well for FOR_HINT honestly, those are really the same
> operations, and FOR_HINT is just here for statistic purposes.

I don't think it's all that much the same operation. And WRT statistics
purpose: Being able to easily differentiate FOR_HINT is important for
pg_xlogdump --stats, but not at all for XLOG_FPI_FLUSH.

> >> --- a/src/backend/access/transam/xloginsert.c
> >> +++ b/src/backend/access/transam/xloginsert.c
> >> @@ -932,10 +932,13 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std)
> >> * If the page follows the standard page layout, with a PageHeader and unused
> >> * space between pd_lower and pd_upper, set 'page_std' to TRUE. That allows
> >> * the unused space to be left out from the WAL record, making it smaller.
> >> + *
> >> + * If 'is_flush' is set to TRUE, relation will be requested to flush
> >> + * immediately its data at replay after replaying this full page image.
> >> */
> >
> > s/is_flush/flush_immed/? And maybe say that it 'will be flushed to the
> > OS immediately after replaying the record'?
>
> s/OS/stable storage?

It's not flushed to stable storage here. Just to the OS.

> diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
> index 99337b0..fff48ab 100644
> --- a/src/backend/access/brin/brin.c
> +++ b/src/backend/access/brin/brin.c
> @@ -682,7 +682,15 @@ brinbuildempty(PG_FUNCTION_ARGS)
> brin_metapage_init(BufferGetPage(metabuf), BrinGetPagesPerRange(index),
> BRIN_CURRENT_VERSION);
> MarkBufferDirty(metabuf);
> - log_newpage_buffer(metabuf, false);
> +
> + /*
> + * For unlogged relations, this page should be immediately flushed
> + * to disk after being replayed. This is necessary to ensure that the
> + * initial on-disk state of unlogged relations is preserved as the
> + * on-disk files are copied directly at the end of recovery.
> + */
> + log_newpage_buffer(metabuf, false,
> + index->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
> END_CRIT_SECTION();
>
> UnlockReleaseBuffer(metabuf);
> diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
> index f876f62..572fe20 100644
> --- a/src/backend/access/brin/brin_pageops.c
> +++ b/src/backend/access/brin/brin_pageops.c
> @@ -865,7 +865,7 @@ brin_initialize_empty_new_buffer(Relation idxrel, Buffer buffer)
> page = BufferGetPage(buffer);
> brin_page_init(page, BRIN_PAGETYPE_REGULAR);
> MarkBufferDirty(buffer);
> - log_newpage_buffer(buffer, true);
> + log_newpage_buffer(buffer, true, false);
> END_CRIT_SECTION();
>
> /*
> diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
> index 49e9185..17c168a 100644
> --- a/src/backend/access/gin/gininsert.c
> +++ b/src/backend/access/gin/gininsert.c
> @@ -450,14 +450,22 @@ ginbuildempty(PG_FUNCTION_ARGS)
> ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
> LockBuffer(RootBuffer, BUFFER_LOCK_EXCLUSIVE);
>
> - /* Initialize and xlog metabuffer and root buffer. */
> + /*
> + * Initialize and xlog metabuffer and root buffer. For unlogged
> + * relations, those pages need to be immediately flushed to disk
> + * after being replayed. This is necessary to ensure that the
> + * initial on-disk state of unlogged relations is preserved when
> + * they get reset at the end of recovery.
> + */
> START_CRIT_SECTION();
> GinInitMetabuffer(MetaBuffer);
> MarkBufferDirty(MetaBuffer);
> - log_newpage_buffer(MetaBuffer, false);
> + log_newpage_buffer(MetaBuffer, false,
> + index->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
> GinInitBuffer(RootBuffer, GIN_LEAF);
> MarkBufferDirty(RootBuffer);
> - log_newpage_buffer(RootBuffer, false);
> + log_newpage_buffer(RootBuffer, false,
> + index->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
> END_CRIT_SECTION();
>
> /* Unlock and release the buffers. */
> diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
> index 53bccf6..6a20031 100644
> --- a/src/backend/access/gist/gist.c
> +++ b/src/backend/access/gist/gist.c
> @@ -84,7 +84,8 @@ gistbuildempty(PG_FUNCTION_ARGS)
> START_CRIT_SECTION();
> GISTInitBuffer(buffer, F_LEAF);
> MarkBufferDirty(buffer);
> - log_newpage_buffer(buffer, true);
> + log_newpage_buffer(buffer, true,
> + index->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED);
> END_CRIT_SECTION();
>

I might be missing something here - but isn't it pretty much guaranteed
that all these are unlogged relations? Otherwise *buildempty() wouldn't
have been called, right?

> else if (info == XLOG_BACKUP_END)
> {
> @@ -178,9 +183,6 @@ xlog_identify(uint8 info)
> case XLOG_FPI:
> id = "FPI";
> break;
> - case XLOG_FPI_FOR_HINT:
> - id = "FPI_FOR_HINT";
> - break;
> }

Really don't want to do that.

> @@ -9391,14 +9394,34 @@ xlog_redo(XLogReaderState *record)
> * resource manager needs to generate conflicts, it has to define a
> * separate WAL record type and redo routine.
> *
> - * XLOG_FPI_FOR_HINT records are generated when a page needs to be
> - * WAL- logged because of a hint bit update. They are only generated
> - * 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.
> + * Records flagged with 'for_hint_bits' are generated when a page needs
> + * to be WAL- logged because of a hint bit update. They are only
> + * generated when checksums are enabled. There is no difference in
> + * handling records when this flag is set, it is used for statistics
> + * purposes.
> + *
> + * Records flagged with 'is_flush' 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.
> */
> if (XLogReadBufferForRedo(record, 0, &buffer) != BLK_RESTORED)
> elog(ERROR, "unexpected XLogReadBufferForRedo result when restoring backup block");
> +
> + if (xlrec.is_flush)
> + {
> + RelFileNode rnode;
> + ForkNumber forknum;
> + BlockNumber blkno;
> + SMgrRelation srel;
> +
> + (void) XLogRecGetBlockTag(record, 0, &rnode, &forknum, &blkno);
> + srel = smgropen(rnode, InvalidBackendId);
> + smgrwrite(srel, forknum, blkno, BufferGetPage(buffer), false);
> + smgrclose(srel);
> + }

That'd leave the dirty bit set on the buffer...

Greetings,

Andres Freund