Re: unlogged sequences

Hi,

On 2019-06-20 09:30:34 +0200, Peter Eisentraut wrote:
> I'm looking for feedback from those who have worked on tableam and
> storage manager to see what the right interfaces are or whether some new
> interfaces might perhaps be appropriate.

Hm, it's not clear to me that tableam design matters much around
sequences? To me it's a historical accident that sequences kinda look
like tables, not more.

> + /*
> + * create init fork for unlogged sequences
> + *
> + * The logic follows that of RelationCreateStorage() and
> + * RelationCopyStorage().
> + */
> + if (seq->sequence->relpersistence == RELPERSISTENCE_UNLOGGED)
> + {
> + SMgrRelation srel;
> + PGAlignedBlock buf;
> + Page page = (Page) buf.data;
> +
> + FlushRelationBuffers(rel);

That's pretty darn expensive, especially when we just need to flush out
a *single* page, as it needs to scan all of shared buffers. Seems better
to just to initialize the page from scratch? Any reason not to do that?

> + srel = smgropen(rel->rd_node, InvalidBackendId);
> + smgrcreate(srel, INIT_FORKNUM, false);
> + log_smgrcreate(&rel->rd_node, INIT_FORKNUM);
> +
> + Assert(smgrnblocks(srel, MAIN_FORKNUM) == 1);
> +
> + smgrread(srel, MAIN_FORKNUM, 0, buf.data);
> +
> + if (!PageIsVerified(page, 0))
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("invalid page in block %u of relation %s",
> + 0,
> + relpathbackend(srel->smgr_rnode.node,
> + srel->smgr_rnode.backend,
> + MAIN_FORKNUM))));
> +
> + log_newpage(&srel->smgr_rnode.node, INIT_FORKNUM, 0, page, false);
> + PageSetChecksumInplace(page, 0);
> + smgrextend(srel, INIT_FORKNUM, 0, buf.data, false);
> + smgrclose(srel);
> + }

I.e. I think it'd be better if we just added a fork argument to
fill_seq_with_data(), and then do something like

smgrcreate(srel, INIT_FORKNUM, false);
log_smgrcreate(&rel->rd_node, INIT_FORKNUM);
fill_seq_with_data(rel, tuple, INIT_FORKNUM);

and add a FlushBuffer() to the end of fill_seq_with_data() if writing
INIT_FORKNUM. The if (RelationNeedsWAL(rel)) would need an || forkNum ==
INIT_FORKNUM.

Alternatively you could just copy the contents from the buffer currently
filled in fill_seq_with_data() to the main fork, and do a memcpy. But
that seems unnecessarily complicated, because you'd again need to do WAL
logging etc.

Greetings,

Andres Freund