Re: Replication identifiers, take 4

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).

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.

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?

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

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.

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

There are no docs for the new sql interfaces.

The replication_identifier.c might deserve some intro/notes text.

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