Thread: Hot Standby, max_connections and max_prepared_transactions

We discussed earlier that HS should continue to work even if
max_connections was set differently on the primary and the standby. This
now gives a situation where snapshots can be allowed, then disallowed
for a while, then allowed again.

Complication is that this will cause some connections to fail since we
take a snapshot in postinit.c. (That is the part I just noticed in my
self-review). Some queries will also fail. Sometimes, not all the time.

This makes both behaviour and coding more complicated and my feeling is
that if we are aiming for simplicity in all areas we should remove
this. 

Currently max_prepared_transactions needs to be set correctly in
recovery also, so this complex coding doesn't actually remove the need
to set some parameters correctly. Not many people change them from the
default in the first place, so I don't think its a big deal. And most
people use the same postgresql.conf on the standby anyway.

I propose we just accept that both max_connections and
max_prepared_transactions need to be set correctly for recovery to work.
This will make the state transitions more robust and it will avoid
spurious and hard to test error messages.

Any objections to me removing this slice of code from the patch? 

-- Simon Riggs           www.2ndQuadrant.com

Simon Riggs wrote:
> I propose we just accept that both max_connections and
> max_prepared_transactions need to be set correctly for recovery to work.
> This will make the state transitions more robust and it will avoid
> spurious and hard to test error messages.
> 
> Any objections to me removing this slice of code from the patch? 

Umm, what slice of code? I don't recall any code trying to make it work.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

On Thu, 2009-09-03 at 22:22 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > I propose we just accept that both max_connections and
> > max_prepared_transactions need to be set correctly for recovery to work.
> > This will make the state transitions more robust and it will avoid
> > spurious and hard to test error messages.
> > 
> > Any objections to me removing this slice of code from the patch? 
> 
> Umm, what slice of code? I don't recall any code trying to make it work.

Well, its there. Perhaps the full functionality has been clipped in
recent changes, but there are still unwanted ramifications in the design
that I think would be best to remove. No loss of functionality, just HS
won't activate unless max_connections is set >= value on primary.

Since max_prepared_transactions already has this same problem in the
current code I see no reason to fuss. We can always put in more flexible
code later.

State change code in StartupXLog(), Snapshots disabled code in
GetSnapshotData(), ProcArray->allowStandbySnapshots etc..

-- Simon Riggs           www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Thu, 2009-09-03 at 22:22 +0300, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>>> I propose we just accept that both max_connections and
>>> max_prepared_transactions need to be set correctly for recovery to work.
>>> This will make the state transitions more robust and it will avoid
>>> spurious and hard to test error messages.
>>> Any objections to me removing this slice of code from the patch? 

>> Umm, what slice of code? I don't recall any code trying to make it work.

> Well, its there.

Just to be clear: you're proposing requiring that these be set the
same on master and slave?  I don't have a problem with that, but
I do suggest that we must provide a mechanism to check it --- I don't
want DBAs to be faced with obscure failures when (not if) they
mess it up.  Perhaps include the values in checkpoint WAL records?
        regards, tom lane

On Fri, 2009-09-04 at 01:25 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Thu, 2009-09-03 at 22:22 +0300, Heikki Linnakangas wrote:
> >> Simon Riggs wrote:
> >>> I propose we just accept that both max_connections and
> >>> max_prepared_transactions need to be set correctly for recovery to work.
> >>> This will make the state transitions more robust and it will avoid
> >>> spurious and hard to test error messages.
> >>> Any objections to me removing this slice of code from the patch? 
> 
> >> Umm, what slice of code? I don't recall any code trying to make it work.
> 
> > Well, its there.
> 
> Just to be clear: you're proposing requiring that these be set the
> same on master and slave?  

Yes, or more precisely: Slave value >= Master value

> I don't have a problem with that, but
> I do suggest that we must provide a mechanism to check it --- I don't
> want DBAs to be faced with obscure failures when (not if) they
> mess it up.  Perhaps include the values in checkpoint WAL records?

Good plan. We can generate an immediate message at startup.

-- Simon Riggs           www.2ndQuadrant.com

On Thu, Sep 3, 2009 at 5:50 PM, Simon Riggs<simon@2ndquadrant.com> wrote:
>
> On Thu, 2009-09-03 at 22:22 +0300, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>> > I propose we just accept that both max_connections and
>> > max_prepared_transactions need to be set correctly for recovery to work.
>> > This will make the state transitions more robust and it will avoid
>> > spurious and hard to test error messages.
>> >
>> > Any objections to me removing this slice of code from the patch?
>>
>> Umm, what slice of code? I don't recall any code trying to make it work.
>
> Well, its there. Perhaps the full functionality has been clipped in
> recent changes, but there are still unwanted ramifications in the design
> that I think would be best to remove. No loss of functionality, just HS
> won't activate unless max_connections is set >= value on primary.

I'm not sure if you're referring to the work that I did, but if so
it's not nearly that significant.  I haven't done anything much beyond
some code cleanup.  Hopefully you're planning to keep that part, as it
would be a shame if I had done it for nothing.  Can't wait to see the
revised patch.

...Robert

On Fri, 2009-09-04 at 09:26 -0400, Robert Haas wrote:
> On Thu, Sep 3, 2009 at 5:50 PM, Simon Riggs<simon@2ndquadrant.com> wrote:
> >
> > On Thu, 2009-09-03 at 22:22 +0300, Heikki Linnakangas wrote:
> >> Simon Riggs wrote:
> >> > I propose we just accept that both max_connections and
> >> > max_prepared_transactions need to be set correctly for recovery to work.
> >> > This will make the state transitions more robust and it will avoid
> >> > spurious and hard to test error messages.
> >> >
> >> > Any objections to me removing this slice of code from the patch?
> >>
> >> Umm, what slice of code? I don't recall any code trying to make it work.
> >
> > Well, its there. Perhaps the full functionality has been clipped in
> > recent changes, but there are still unwanted ramifications in the design
> > that I think would be best to remove. No loss of functionality, just HS
> > won't activate unless max_connections is set >= value on primary.
> 
> I'm not sure if you're referring to the work that I did, but if so
> it's not nearly that significant. 

No, this was introduced in Jan/Feb.

-- Simon Riggs           www.2ndQuadrant.com

On Fri, 2009-09-04 at 09:26 -0400, Robert Haas wrote:

> Hopefully you're planning to keep that part, as it
> would be a shame if I had done it for nothing. 

Not promising anything in that regard, but I appreciate your efforts to
assist. I doubt it was wasted effort in any sense. It will certainly be
valuable to have another knowledgeable reviewer.

I'm finding reviewing my own code considerably easier after the break in
development. Not done yet, but making good progress every day.

-- Simon Riggs           www.2ndQuadrant.com