Re: unlogged sequences - Mailing list pgsql-hackers

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