Re: Replication identifiers, take 4

On 2015-02-22 04:59:30 +0100, Petr Jelinek wrote:
> Now that the issue with padding seems to no longer exists since the patch
> works both with and without padding, I went through the code and here are
> some comments I have (in no particular order).
>
> In CheckPointReplicationIdentifier:
> >+ * FIXME: Add a CRC32 to the end.
>
> The function already does it (I guess you forgot to remove the comment).

Yep. I locally have a WIP version that's much cleaned up and doesn't
contain it anymore.

> Using max_replication_slots as limit for replication_identifier states does
> not really make sense to me as replication_identifiers track remote info
> while and slots are local and in case of master-slave replication you need
> replication identifiers but don't need slots.

On the other hand, it's quite cheap if unused. Not sure if several
variables are worth it.

> In bootstrap.c:
> > #define MARKNOTNULL(att) \
> > ((att)->attlen > 0 || \
> > (att)->atttypid == OIDVECTOROID || \
> >- (att)->atttypid == INT2VECTOROID)
> >+ (att)->atttypid == INT2VECTOROID || \
> >+ strcmp(NameStr((att)->attname), "riname") == 0 \
> >+ )
>
> Huh? Can this be solved in a nicer way?

Yes. I'd mentioned that this is just a temporary hack ;). I've since
pushed a more proper solution; BKI_FORCE_NOT_NULL can now be specified
on the column.

> Since we call XLogFlush with local_lsn as parameter, shouldn't we check that
> it's actually within valid range?
> Currently we'll get errors like this if set to invalid value:
> ERROR: xlog flush request 123/123 is not satisfied --- flushed only to
> 0/168FB18

I think we should rather remove local_lsn from all parameters that are
user callable, adding them there was a mistake. It's really only
relevant for the cases where it's called by commit.

> In AdvanceReplicationIndentifier:
> >+ /*
> >+ * XXX: should we restore into a hashtable and dump into shmem only after
> >+ * recovery finished?
> >+ */
>
> Probably no given that the function is also callable via SQL interface.

Well, it's still a good idea regardless...

> As I wrote in another email, I would like to integrate the RepNodeId and
> CommitTSNodeId into single thing.

Will reply separately there.

> There are no docs for the new sql interfaces.

Yea. The whole thing needs docs.

Thanks,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services