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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
