Horton

Thread

Commits

  1. Consistently declare timestamp variables as TimestampTz.

  2. Add pg_recvlogical —-endpos=LSN

  1. Keeping pg_recvlogical's "feTimestamp" separate from TimestampTz

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-17T18:15:38Z

    Thomas Munro pointed out that commit 7c030783a broke things on
--disable-integer-datetimes builds, because somebody cleverly used
TimestampTz to declare timestamp variables, no doubt not having
read the comment (which doesn't even appear in the same file :-()
that

 * ... The replication protocol always uses integer timestamps,
 * regardless of the server setting.

I am not sure that it was really a good design to pretend that the
replication protocol is independent of --disable-integer-datetimes
when the underlying WAL stream most certainly isn't.

However, if we keep it like this, I think we need to expend significantly
more effort on making sure that "feTimestamps" aren't confused with
TimestampTzs.  It's only the sheer happenstance that a couple of new
functions were declared with "TimestampTz *" not "TimestampTz" arguments
that we found this mechanically at all --- otherwise, the compiler doesn't
know any better than to naively cast from int64 to double or vice versa
when asked to.

Moreover, I think there is absolutely nothing stopping somebody from
trying to compare a replication-protocol timestamp to a TimestampTz
taken out of the WAL stream, which will give completely wrong answers
for float timestamps, and which no compiler on earth will warn about.
Even if there are no such occurrences today, does anyone really want
to bet that somebody won't submit a patch tomorrow that subtly depends
on replication-protocol timestamps matching backend timestamps?

I propose that what we need to do is get rid of the dangerous and
none-too-readable-anyway use of "int64" to declare replication-protocol
timestamps, and instead declare them as, say,

	typedef struct RPTimestamp
	{
	    int64 rptimestamp;
	} RPTimestamp;

This will entail slightly more notation in the subroutines that actually
do something with the timestamp values, but it will make certain that
you can't assign or compare an RPTimestamp to a backend timestamp without
the compiler complaining about it.

			regards, tom lane

  2. Re: Keeping pg_recvlogical's "feTimestamp" separate from TimestampTz

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-18T22:18:44Z

    I wrote:
> I propose that what we need to do is get rid of the dangerous and
> none-too-readable-anyway use of "int64" to declare replication-protocol
> timestamps, and instead declare them as, say,

> 	typedef struct RPTimestamp
> 	{
> 	    int64 rptimestamp;
> 	} RPTimestamp;

I experimented with this a bit, as in the attached draft patch, and
concluded that use of a struct is probably a bridge too far.  It makes the
patch pretty invasive notationally, and yet it still fails to catch places
where nobody had bothered to even think about float timestamps anywhere
nearby; which there are several of, as per my report
https://www.postgresql.org/message-id/26788.1487455319@sss.pgh.pa.us

We might be able to salvage the parts of this that simply redeclare
appropriate variables as "IntegerTimestamp" rather than "int64", with
the type declaration just being "typedef int64 IntegerTimestamp".
If we go forward with keeping protocol fields as integer timestamps
then I'd want to do that, just so we have some notation in the code
as to what the values are meant to be.  But right at the moment I'm
thinking we'd be better off to change them all to TimestampTz instead,
and adjust the arithmetic on them appropriately.

			regards, tom lane