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