unlogged sequences

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

unlogged sequences

Peter Eisentraut-6
The discussion in bug #15631 revealed that serial/identity sequences of
temporary tables should really also be temporary (easy), and that
serial/identity sequences of unlogged tables should also be unlogged.
But there is no support for unlogged sequences, so I looked into that.

If you copy the initial sequence relation file to the init fork, then
this all seems to work out just fine.  Attached is a patch.  The
low-level copying seems to be handled quite inconsistently across the
code, so I'm not sure what the most appropriate way to do this would be.
 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.

(What's still missing in this patch is ALTER SEQUENCE SET
{LOGGED|UNLOGGED} as well as propagating the analogous ALTER TABLE
command to owned sequences.)

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v1-0001-Make-command-order-in-test-more-sensible.patch (2K) Download Attachment
v1-0002-Fix-comment.patch (2K) Download Attachment
v1-0003-Unlogged-sequences.patch (12K) Download Attachment
v1-0004-Make-identity-serial-sequences-unlogged-when-thei.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: unlogged sequences

Michael Paquier-2
On Thu, Jun 20, 2019 at 09:30:34AM +0200, Peter Eisentraut wrote:
> The discussion in bug #15631 revealed that serial/identity sequences of
> temporary tables should really also be temporary (easy), and that
> serial/identity sequences of unlogged tables should also be unlogged.
> But there is no support for unlogged sequences, so I looked into that.

Thanks for doing so.

> If you copy the initial sequence relation file to the init fork, then
> this all seems to work out just fine.  Attached is a patch.  The
> low-level copying seems to be handled quite inconsistently across the
> code, so I'm not sure what the most appropriate way to do this would be.
>  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.

But the actual deal is that the sequence meta-data is now in
pg_sequences and not the init forks, no?  I have just done a small
test:
1) Some SQL queries:
create unlogged sequence popo;
alter sequence popo increment 2;
select nextval('popo');
select nextval('popo');
2) Then a hard crash:
pg_ctl stop -m immediate
pg_ctl start
3) Again, with a crash:
select nextval('popo');                                                                                                                                                    
#2  0x000055ce60f3208d in ExceptionalCondition
(conditionName=0x55ce610f0570 "!(((PageHeader) (page))->pd_special >=
(__builtin_offsetof (PageHeaderData, pd_linp)))",
errorType=0x55ce610f0507 "FailedAssertion",
fileName=0x55ce610f04e0 "../../../src/include/storage/bufpage.h",
lineNumber=317) at assert.c:54
#3  0x000055ce60b43200 in PageValidateSpecialPointer
(page=0x7ff7692b3d80 "") at
../../../src/include/storage/bufpage.h:317
#4  0x000055ce60b459d4 in read_seq_tuple (rel=0x7ff768ad27e0,
buf=0x7ffc5707f0bc, seqdatatuple=0x7ffc5707f0a0) at
sequence.c:1213
#5  0x000055ce60b447ee in nextval_internal (relid=16385,
check_permissions=true) at sequence.c:678
#6  0x000055ce60b44533 in nextval_oid (fcinfo=0x55ce62537570) at sequence.c:607
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: unlogged sequences

Peter Eisentraut-6
On 2019-06-21 07:31, Michael Paquier wrote:
> 1) Some SQL queries:
> create unlogged sequence popo;
> alter sequence popo increment 2;

The problem is that the above command does a relation rewrite but the
code doesn't know to copy the init fork of the sequence.  That will need
to be addressed.

> select nextval('popo');
> select nextval('popo');
> 2) Then a hard crash:
> pg_ctl stop -m immediate
> pg_ctl start
> 3) Again, with a crash:
> select nextval('popo');                                                                                                                                                    
> #2  0x000055ce60f3208d in ExceptionalCondition

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: unlogged sequences

Andres Freund
In reply to this post by Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: unlogged sequences

Thomas Munro-5
On Wed, Jun 26, 2019 at 6:38 AM Andres Freund <[hidden email]> wrote:
> 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.
>
> [lots of feedback that requires making decisions]

Seems to be actively under development but no new patch yet.  Moved to next CF.

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: unlogged sequences

Álvaro Herrera
On 2019-Aug-01, Thomas Munro wrote:

> On Wed, Jun 26, 2019 at 6:38 AM Andres Freund <[hidden email]> wrote:
> > 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.
> >
> > [lots of feedback that requires making decisions]
>
> Seems to be actively under development but no new patch yet.  Moved to next CF.

Marked Waiting on Author.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services