Re: Incorrect initialization of sentPtr in walsender.c

Hi all,

In walsender.c, sentPtr is initialized as follows:
static XLogRecPtr sentPtr = 0;
Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?
Patch is attached.
Regards,
--
Michael

On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> In walsender.c, sentPtr is initialized as follows:
> static XLogRecPtr sentPtr = 0;
> Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?
Actually by looking more around I found a couple of extra places where
the same inconsistencies are present, mainly in xlog.c and
walreceiver.c. Updated patch attached for all those things.
Regards,
--
Michael

On 09/12/2014 03:17 AM, Michael Paquier wrote:
> On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> In walsender.c, sentPtr is initialized as follows:
>> static XLogRecPtr sentPtr = 0;
>> Isn't that incorrect and shouldn't we use InvalidXLogRecPtr instead?
> Actually by looking more around I found a couple of extra places where
> the same inconsistencies are present, mainly in xlog.c and
> walreceiver.c. Updated patch attached for all those things.

InvalidXLogRecPtr == 0, so it's just a style issue which one is more
correct.

I haven't looked at those places closely, but it seems possible that at
least some of those variables are supposed to be initialized to a value
smaller than any valid WAL position, rather than just Invalid. In other
words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we
would still want those variables to be initialized to zero. As I said, I
didn't check the code, but before we change those that ought to be checked.

- Heikki

On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> I haven't looked at those places closely, but it seems possible that at
> least some of those variables are supposed to be initialized to a value
> smaller than any valid WAL position, rather than just Invalid. In other
> words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
> still want those variables to be initialized to zero. As I said, I didn't
> check the code, but before we change those that ought to be checked.

Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
in xlog.c need to use the lowest pointer value possible as they do a
couple of comparisons with other positions. This is as well the case
of sentPtr in walsender.c. However, that's not the case of writePtr
and flushPtr in walreceiver.c as those positions are just used for
direct comparison with LogstreamResult, so we could use
InvalidXLogRecPtr in this case.

What do you think of the addition of a #define for the lowest possible
XLOG location pointer? I've wanted that as well a couple of times when
working on clients using replication connections for for example
START_REPLICATION. That would be better than hardcoding a position
like '0/0', and would make the current code more solid.

Patch attached in case.

Regards,
--
Michael

On Fri, Sep 12, 2014 at 09:16:42PM +0900, Michael Paquier wrote:
> On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
> > I haven't looked at those places closely, but it seems possible that at
> > least some of those variables are supposed to be initialized to a value
> > smaller than any valid WAL position, rather than just Invalid. In other
> > words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
> > still want those variables to be initialized to zero. As I said, I didn't
> > check the code, but before we change those that ought to be checked.
>
> Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
> in xlog.c need to use the lowest pointer value possible as they do a
> couple of comparisons with other positions. This is as well the case
> of sentPtr in walsender.c. However, that's not the case of writePtr
> and flushPtr in walreceiver.c as those positions are just used for
> direct comparison with LogstreamResult, so we could use
> InvalidXLogRecPtr in this case.
>
> What do you think of the addition of a #define for the lowest possible
> XLOG location pointer? I've wanted that as well a couple of times when
> working on clients using replication connections for for example
> START_REPLICATION. That would be better than hardcoding a position
> like '0/0', and would make the current code more solid.
>
> Patch attached in case.

I like this. Can we apply it Heikki?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Tue, Oct 14, 2014 at 5:18 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Fri, Sep 12, 2014 at 09:16:42PM +0900, Michael Paquier wrote:
>> Patch attached in case.
> I like this. Can we apply it Heikki?
I actually registered it to the next CF so as it does not fall into
oblivion, simply forgot to mention it:
https://commitfest.postgresql.org/action/patch_view?id=1572
Note that this patch still sets InvalidXLogRecPtr to 0. I think that
it should be INT64_MAX for clarity.
Regards
--
Michael

On 12 September 2014 13:16, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> I haven't looked at those places closely, but it seems possible that at
>> least some of those variables are supposed to be initialized to a value
>> smaller than any valid WAL position, rather than just Invalid. In other
>> words, if we defined InvalidXLogRecPtr as INT64_MAX rather than 0, we would
>> still want those variables to be initialized to zero. As I said, I didn't
>> check the code, but before we change those that ought to be checked.
>
> Ah, OK. I just had a look at that, and receivedUpto and lastComplaint
> in xlog.c need to use the lowest pointer value possible as they do a
> couple of comparisons with other positions. This is as well the case
> of sentPtr in walsender.c. However, that's not the case of writePtr
> and flushPtr in walreceiver.c as those positions are just used for
> direct comparison with LogstreamResult, so we could use
> InvalidXLogRecPtr in this case.

I don't see this patch gives us anything. All it will do is prevent
easy backpatching of related fixes.

-1 for changing the code in this kind of way

I find it confusing that the "Lowest" pointer value is also "Invalid".
Valid != Invalid

-1 for this patch

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

On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> I find it confusing that the "Lowest" pointer value is also "Invalid".
> Valid != Invalid
>
In complement to that, note that I mentioned Invalid should be UINT_MAX for
clarity.
--
Michael

On Thu, Oct 16, 2014 at 9:01 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
>
>> I find it confusing that the "Lowest" pointer value is also "Invalid".
>> Valid != Invalid
>>
> In complement to that, note that I mentioned Invalid should be UINT_MAX
> for clarity.
>
Worth noticing that this patch has been marked as returned with feedback.
--
Michael