Thread: 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
Attachment
On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier <michael.paquier@gmail.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
Attachment
On 09/12/2014 03:17 AM, Michael Paquier wrote: > On Fri, Sep 12, 2014 at 9:08 AM, Michael Paquier > <michael.paquier@gmail.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@vmware.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
Attachment
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@vmware.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@momjian.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@momjian.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.paquier@gmail.com> wrote: > On Fri, Sep 12, 2014 at 4:55 PM, Heikki Linnakangas > <hlinnakangas@vmware.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@2ndquadrant.com> wrote:
--
Michael
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.paquier@gmail.com> wrote:
Worth noticing that this patch has been marked as returned with feedback.On Thu, Oct 16, 2014 at 7:09 PM, Simon Riggs <simon@2ndquadrant.com> wrote:I find it confusing that the "Lowest" pointer value is also "Invalid".
Valid != InvalidIn complement to that, note that I mentioned Invalid should be UINT_MAX for clarity.
--
Michael