Re: Hot Standby, release candidate?

Enclose latest version of Hot Standby. This is the "basic" patch, with
no must-fix issues and no known bugs. Further additions will follow
after commit, primarily around usability, which will include additional
control functions for use in testing. Various thoughts discussed here
http://wiki.postgresql.org/wiki/Hot_Standby_TODO

Patch now includes a set of regression tests that can be run against a
standby server using "make standbycheck" (requires setup, see
src/test/regress/standby_schedule). This helps make explicit in code
which commands and variants are allowed and disallowed in hot standby.

Complete with full docs and comments.

Barring resolving a few points and subject to even more testing, this is
the version I expect to commit to CVS on Wednesday. I would appreciate
further objective testing before commit, if possible.

Last remaining points

* NonTransactionalInvalidation logging has been removed following
review, but AFAICS that means VACUUM FULL doesn't work correctly on
catalog tables, which regrettably will be the only ones still standing
even after we apply VFI patch. Did I misunderstand the original intent?
Was it just buggy somehow? Or is this hoping VF goes completely, which
seems unlikely in this release. Just noticed this, decided better to get
stuff out there now.

* Are we OK with using hash indexes in standby plans, even when we know
the indexes are stale and could return incorrect results?

Software also available via git on the repo used by Heikki and myself.
ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git
branch = hs-riggs

Patch prepared using from my private repo using
git diff pgsql/master..hs-simon | filterdiff --format=context
so please look out for any weirdness that might introduce.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> * NonTransactionalInvalidation logging has been removed following
> review, but AFAICS that means VACUUM FULL doesn't work correctly on
> catalog tables, which regrettably will be the only ones still standing
> even after we apply VFI patch. Did I misunderstand the original intent?
> Was it just buggy somehow? Or is this hoping VF goes completely, which
> seems unlikely in this release.

For my money, the only reason VF is still around is there hasn't been
an urgent reason to get rid of it. If it doesn't play with HS, I think
we'd be better served to put work into getting rid of it than to put
work into fixing it.

regards, tom lane

On Sun, 2009-12-13 at 15:45 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > * NonTransactionalInvalidation logging has been removed following
> > review, but AFAICS that means VACUUM FULL doesn't work correctly on
> > catalog tables, which regrettably will be the only ones still standing
> > even after we apply VFI patch. Did I misunderstand the original intent?
> > Was it just buggy somehow? Or is this hoping VF goes completely, which
> > seems unlikely in this release.
>
> For my money, the only reason VF is still around is there hasn't been
> an urgent reason to get rid of it. If it doesn't play with HS, I think
> we'd be better served to put work into getting rid of it than to put
> work into fixing it.

I see the logic, though it has many implications. I'll step up, if I can
get some help from you and Itagaki on the VF side.

You have a rough design here
http://archives.postgresql.org/message-id/19750.1252094460@sss.pgh.pa.us

Some thoughts and some further work on a detailed design

* Which exact tables are we talking about: just pg_class and the shared
catalogs? Everything else is in pg_class, so if we can find it we're OK?
formrdesc() tells me the list of nailed relations is: pg_database,
pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
the ones we care about, or are they just a subset?

* Restrict set of operations to *only* VACUUM FULL. Is there a need for
anything else to do this, at least in this release?

* Each backend needs to access two map files: shared and local

* Get relcache to read map files at startup in formrdesc(). Rather than
use RelationInitPhysicalAddr() set relation->rd_node.relNode directly

* Get VF to write a new type of invalidation message that means re-read
the two map files to overwrite the relation->rd_node.relNode in the
nailed relations

* Map files would have a very structured format, so each table listed
has its exact place. Sounds like best place for shared catalogs is
pg_control. We only need a few additional bytes for that and everything
else to manipulate it already exists.

* Map files for specific databases would be called pg_database_control,
with roughly same concepts as pg_control. It's then an obvious place to
add any further db specific things in future, if we need them.

* Protect all map files reading/writing using ControlFileLock. Sequence
of update is acquire lock, send invalidation, rewrite file, release lock
all inside a critical section. Readers would take shared, writers
exclusive.

* Work would be in two tranches: add new way of working then later
remove code we don't need; I would actually rather do the second part at
start of next dev cycle.

--
Simon Riggs www.2ndQuadrant.com

On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
> Barring resolving a few points and subject to even more testing, this
> is the version I expect to commit to CVS on Wednesday.

To clarify: Did you pick Wednesday so that it gets included before the
end of the commit fest (and thus into alpha3) or so that it gets into
CVS as early as possible after the commit fest (and thus not into
alpha3)?

On Mon, 2009-12-14 at 10:11 +0200, Peter Eisentraut wrote:
> On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
> > Barring resolving a few points and subject to even more testing, this
> > is the version I expect to commit to CVS on Wednesday.
>
> To clarify: Did you pick Wednesday so that it gets included before the
> end of the commit fest (and thus into alpha3) or so that it gets into
> CVS as early as possible after the commit fest (and thus not into
> alpha3)?

Wednesday because that seemed a good delay to allow review. Josh and I
had discussed the value of getting patch into Alpha3, so that was my
wish and aim.

I'm not aware of any particular date for end of commitfest, though
possibly you are about to update me on that?

(Perhaps we really need a single Project Management page that lists all
the dates that have been agreed, so that everybody can go there and be
clear. Commitfest start and end dates, beta dates, de-support dates etc.
BTW, it is absolutely brilliant that we have these now.)

--
Simon Riggs www.2ndQuadrant.com

On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
> Wednesday because that seemed a good delay to allow review. Josh and I
> had discussed the value of getting patch into Alpha3, so that was my
> wish and aim.
>
> I'm not aware of any particular date for end of commitfest, though
> possibly you are about to update me on that?

Commit fests for 8.5 have usually run from the 15th to the 15th of next
month, but the CF manager may choose to vary that.

FWIW, the alpha release manager may also vary the release timeline of
alpha3. ;-)

> (Perhaps we really need a single Project Management page that lists all
> the dates that have been agreed, so that everybody can go there and be
> clear. Commitfest start and end dates, beta dates, de-support dates etc.
> BTW, it is absolutely brilliant that we have these now.)

We had
<http://wiki.postgresql.org/wiki/PostgreSQL_8.4_Development_Plan>, but I
don't think we ever actually agreed on a schedule for 8.5.

Simon Riggs wrote:
> Enclose latest version of Hot Standby. This is the "basic" patch, with
> no must-fix issues and no known bugs. Further additions will follow
> after commit, primarily around usability, which will include additional
> control functions for use in testing. Various thoughts discussed here
> http://wiki.postgresql.org/wiki/Hot_Standby_TODO

I still consider it highly important to be able to start standby from a
shutdown checkpoint. If you remove it, at the very least put it back on
the TODO.

But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
changes to PrescanPreparedTransactions() are not needed either.

> Patch now includes a set of regression tests that can be run against a
> standby server using "make standbycheck"

Nice!

> (requires setup, see src/test/regress/standby_schedule).

Any chance of automating that?

> Complete with full docs and comments.
>
> Barring resolving a few points and subject to even more testing, this is
> the version I expect to commit to CVS on Wednesday. I would appreciate
> further objective testing before commit, if possible.

* Please remove any spurious whitespace. "git diff --color" makes them
stand out like a sore thumb, in red. (pgindent will fix them but always
better to fix them before committing, IMO).

* vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
on your transaction rate to begin with. Do we really want this setting
in its current form? Does it make sense as PGC_USERSET, as if one
backend uses a lower setting than others, that's the one that really
determines when transactions are killed in the standby? I think this
should be discussed and implemented as a separate patch.

* Are you planning to remove the recovery_connections setting before
release? The documentation makes it sound like it's a temporary hack
that we're not really sure is needed at all. That's not very comforting.

* You removed this comment from KnownAssignedXidsInit:

- /*
- * XXX: We should check that we don't exceed maxKnownAssignedXids.
- * Even though the hash table might hold a few more entries than that,
- * we use fixed-size arrays of that size elsewhere and expected all
- * entries in the hash table to fit.
- */

but AFAICS you didn't address the issue. It's referring to the 'xids'
array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
in without checking that it fits.

* LockAcquireExtended needs a function comment. Or at least something
explaining what report_memory_error does. And perhaps the argument
should be named "reportMemoryError" to be in line with the other arguments.

* We tend to not add remarks about authors in code (comments in standby.c).

* This optimization in GetConflictingVirtualXIDs():

> + /*
> + * If we don't know the TransactionId that created the conflict, set
> + * it to latestCompletedXid which is the latest possible value.
> + */
> + if (!TransactionIdIsValid(limitXmin))
> + limitXmin = ShmemVariableCache->latestCompletedXid;
> +

needs a lot more explanation. It took me a very long time to figure out
why using latest completed xid is safe.

* Are you going to leave the trace_recovery GUC in?

* Can you merge with CVS HEAD, please? There's some merge conflicts.

> Last remaining points
>
> * NonTransactionalInvalidation logging has been removed following
> review, but AFAICS that means VACUUM FULL doesn't work correctly on
> catalog tables, which regrettably will be the only ones still standing
> even after we apply VFI patch. Did I misunderstand the original intent?
> Was it just buggy somehow? Or is this hoping VF goes completely, which
> seems unlikely in this release. Just noticed this, decided better to get
> stuff out there now.

I removed it in the hope that VF is gone before beta.

> * Are we OK with using hash indexes in standby plans, even when we know
> the indexes are stale and could return incorrect results?

It doesn't seem any more wrong than using hash indexes right after
recovery, crash recovery or otherwise. It's certainly broken, but I
don't see much value in a partial fix. The bottom line is that hash
indexes should be WAL-logged.

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

On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> * Please remove any spurious whitespace.  "git diff --color" makes them
> stand out like a sore thumb, in red. (pgindent will fix them but always
> better to fix them before committing, IMO).

+1 in general, not particularly for this patch (haven't checked that
in this patch).

Actually, how about we add that to the page at
http://wiki.postgresql.org/wiki/Submitting_a_Patch?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Thanks for the further review, much appreciated.

On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Enclose latest version of Hot Standby. This is the "basic" patch, with
> > no must-fix issues and no known bugs. Further additions will follow
> > after commit, primarily around usability, which will include additional
> > control functions for use in testing. Various thoughts discussed here
> > http://wiki.postgresql.org/wiki/Hot_Standby_TODO
>
> I still consider it highly important to be able to start standby from a
> shutdown checkpoint. If you remove it, at the very least put it back on
> the TODO.

Happy to put it back on TODO, but I'm not likely to do this without a
good reason. IMHO your arguments as to why that is useful were not
convincing, especially when it introduces further complications and
requirements for tests.

> But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
> changes to PrescanPreparedTransactions() are not needed either.
>
> > Patch now includes a set of regression tests that can be run against a
> > standby server using "make standbycheck"
>
> Nice!
>
> > (requires setup, see src/test/regress/standby_schedule).
>
> Any chance of automating that?

Future, yes.

I view standbycheck as similar to installcheck - it requires some setup
before it can run, so allows you to test an existing server. I see the
same need here.

> > Complete with full docs and comments.
> >
> > Barring resolving a few points and subject to even more testing, this is
> > the version I expect to commit to CVS on Wednesday. I would appreciate
> > further objective testing before commit, if possible.
>
> * Please remove any spurious whitespace. "git diff --color" makes them
> stand out like a sore thumb, in red. (pgindent will fix them but always
> better to fix them before committing, IMO).

What is your definition of spurious whitespace? I removed all
additions/deletions of individual lines. git diff colours many things
red, and it seems like a waste of time to spend hours fiddling with
spaces manually if there is a utility that does this anyway.

> * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
> on your transaction rate to begin with. Do we really want this setting
> in its current form? Does it make sense as PGC_USERSET, as if one
> backend uses a lower setting than others, that's the one that really
> determines when transactions are killed in the standby? I think this
> should be discussed and implemented as a separate patch.

All the vacuum_*_age parameters have this characteristic, yet they
exist.

I would like to provide simple features for conflict avoidance in the
first alpha release. If we find that nobody found it useful then it can
be removed easily enough.

It's a USERSET now, but it could also be other things. This patch isn't
the end of discussion, for many people it will be the start.

> * Are you planning to remove the recovery_connections setting before
> release? The documentation makes it sound like it's a temporary hack
> that we're not really sure is needed at all. That's not very comforting.

It has been requested and I agree, so its there. Saying it might be
removed in future is no more than we do elsewhere and AFAIK we all hope
it will be. Not sure why that is or isn't comforting.

> * You removed this comment from KnownAssignedXidsInit:
>
> - /*
> - * XXX: We should check that we don't exceed maxKnownAssignedXids.
> - * Even though the hash table might hold a few more entries than that,
> - * we use fixed-size arrays of that size elsewhere and expected all
> - * entries in the hash table to fit.
> - */
>
> but AFAICS you didn't address the issue. It's referring to the 'xids'
> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
> in without checking that it fits.

I have ensured that they are always the same size, by definition, so no
need to check.

> * LockAcquireExtended needs a function comment. Or at least something
> explaining what report_memory_error does. And perhaps the argument
> should be named "reportMemoryError" to be in line with the other arguments.

OK

> * We tend to not add remarks about authors in code (comments in standby.c).

OK

> * This optimization in GetConflictingVirtualXIDs():
>
> > + /*
> > + * If we don't know the TransactionId that created the conflict, set
> > + * it to latestCompletedXid which is the latest possible value.
> > + */
> > + if (!TransactionIdIsValid(limitXmin))
> > + limitXmin = ShmemVariableCache->latestCompletedXid;
> > +
>
> needs a lot more explanation. It took me a very long time to figure out
> why using latest completed xid is safe.

OK. Took me a long time as well.

> * Are you going to leave the trace_recovery GUC in?

For now, at least. I have a later proposal around that to follow.

> * Can you merge with CVS HEAD, please? There's some merge conflicts.

Huh? I did. And tested patch against a CVS checkout before submitting.
Can you explain further?

> > Last remaining points
> >
> > * NonTransactionalInvalidation logging has been removed following
> > review, but AFAICS that means VACUUM FULL doesn't work correctly on
> > catalog tables, which regrettably will be the only ones still standing
> > even after we apply VFI patch. Did I misunderstand the original intent?
> > Was it just buggy somehow? Or is this hoping VF goes completely, which
> > seems unlikely in this release. Just noticed this, decided better to get
> > stuff out there now.
>
> I removed it in the hope that VF is gone before beta.

OK

> > * Are we OK with using hash indexes in standby plans, even when we know
> > the indexes are stale and could return incorrect results?
>
> It doesn't seem any more wrong than using hash indexes right after
> recovery, crash recovery or otherwise. It's certainly broken, but I
> don't see much value in a partial fix. The bottom line is that hash
> indexes should be WAL-logged.

I know that's your thought; I'm just checking its everyone else's as
well. We go to great lengths elsewhere in the patch to avoid queries
returning incorrect results and there is a loss of capability as a
result. I don't want Hash index users to view this as a feature. I don't
feel too strongly, but it can be argued both ways, at least.

--
Simon Riggs www.2ndQuadrant.com

On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> > * Please remove any spurious whitespace. "git diff --color" makes them
> > stand out like a sore thumb, in red. (pgindent will fix them but always
> > better to fix them before committing, IMO).
>
> +1 in general, not particularly for this patch (haven't checked that
> in this patch).
>
> Actually, how about we add that to the page at
> http://wiki.postgresql.org/wiki/Submitting_a_Patch?

If we can define "spurious whitespace" it would help decide whether
there is any action to take, and when.

--
Simon Riggs www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 8:07 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> * Please remove any spurious whitespace.  "git diff --color" makes them
>> stand out like a sore thumb, in red. (pgindent will fix them but always
>> better to fix them before committing, IMO).
>
> What is your definition of spurious whitespace? I removed all
> additions/deletions of individual lines. git diff colours many things
> red, and it seems like a waste of time to spend hours fiddling with
> spaces manually if there is a utility that does this anyway.

I guess it's a trailing whitespace. How about using "git diff --check"
instead of "--color"?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> > * Please remove any spurious whitespace.  "git diff --color" makes them
>> > stand out like a sore thumb, in red. (pgindent will fix them but always
>> > better to fix them before committing, IMO).
>>
>> +1 in general, not particularly for this patch (haven't checked that
>> in this patch).
>>
>> Actually, how about we add that to the page at
>> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>
> If we can define "spurious whitespace" it would help decide whether
> there is any action to take, and when.

git defines it as either (1) extra whitespace at the end of a line or
(2) an initial indent that uses spaces followed by tabs (typically
something like space-tab, where tab alone would have produced the same
result). git diff --check master tends to be useful here.

...Robert

On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> >> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> >> > * Please remove any spurious whitespace. "git diff --color" makes them
> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
> >> > better to fix them before committing, IMO).
> >>
> >> +1 in general, not particularly for this patch (haven't checked that
> >> in this patch).
> >>
> >> Actually, how about we add that to the page at
> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
> >
> > If we can define "spurious whitespace" it would help decide whether
> > there is any action to take, and when.
>
> git defines it as either (1) extra whitespace at the end of a line or
> (2) an initial indent that uses spaces followed by tabs (typically
> something like space-tab, where tab alone would have produced the same
> result). git diff --check master tends to be useful here.

(2) is a problem that has been discussed before on hackers, anything
like that should be changed.

Why is (1) important, and if it is important, why is it being mentioned
only now? Are we saying that all previous reviewers of my work (and
others') removed these without ever mentioning they had done so?

--
Simon Riggs www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
>> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>> >> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
>> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
>> >> > better to fix them before committing, IMO).
>> >>
>> >> +1 in general, not particularly for this patch (haven't checked that
>> >> in this patch).
>> >>
>> >> Actually, how about we add that to the page at
>> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>> >
>> > If we can define "spurious whitespace" it would help decide whether
>> > there is any action to take, and when.
>>
>> git defines it as either (1) extra whitespace at the end of a line or
>> (2) an initial indent that uses spaces followed by tabs (typically
>> something like space-tab, where tab alone would have produced the same
>> result).  git diff --check master tends to be useful here.
>
> (2) is a problem that has been discussed before on hackers, anything
> like that should be changed.
>
> Why is (1) important, and if it is important, why is it being mentioned
> only now? Are we saying that all previous reviewers of my work (and
> others') removed these without ever mentioning they had done so?

pgident will remove such white spaces and create merge conflicts for
everyone working on those areas of the code. I certainly mention this
in any review I do where it's applicable, and have been doing so for
some time. I also will certainly fix it for any code I commit. I
also mentioned it in the review that I did of Hot Standby.

...Robert

On Mon, Dec 14, 2009 at 12:51, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
>>> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>>> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>>> >> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
>>> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
>>> >> > better to fix them before committing, IMO).
>>> >>
>>> >> +1 in general, not particularly for this patch (haven't checked that
>>> >> in this patch).
>>> >>
>>> >> Actually, how about we add that to the page at
>>> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>>> >
>>> > If we can define "spurious whitespace" it would help decide whether
>>> > there is any action to take, and when.
>>>
>>> git defines it as either (1) extra whitespace at the end of a line or
>>> (2) an initial indent that uses spaces followed by tabs (typically
>>> something like space-tab, where tab alone would have produced the same
>>> result).  git diff --check master tends to be useful here.
>>
>> (2) is a problem that has been discussed before on hackers, anything
>> like that should be changed.
>>
>> Why is (1) important, and if it is important, why is it being mentioned
>> only now? Are we saying that all previous reviewers of my work (and
>> others') removed these without ever mentioning they had done so?
>
> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code.  I certainly mention this
> in any review I do where it's applicable, and have been doing so for
> some time.  I also will certainly fix it for any code I commit.  I
> also mentioned it in the review that I did of Hot Standby.

I also always do this when committing other peoples patches (which I
don't do as often as I should, but when I *do* do it..)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Mon, 2009-12-14 at 11:07 +0000, Simon Riggs wrote:
> Thanks for the further review, much appreciated.
>
> On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
> > Simon Riggs wrote:
> > > Enclose latest version of Hot Standby.
>
> > * LockAcquireExtended needs a function comment. Or at least something
> > explaining what report_memory_error does. And perhaps the argument
> > should be named "reportMemoryError" to be in line with the other arguments.
>
> OK

Done

> > * We tend to not add remarks about authors in code (comments in standby.c).
>
> OK

Done

> > * This optimization in GetConflictingVirtualXIDs():
> >
> > > + /*
> > > + * If we don't know the TransactionId that created the conflict, set
> > > + * it to latestCompletedXid which is the latest possible value.
> > > + */
> > > + if (!TransactionIdIsValid(limitXmin))
> > > + limitXmin = ShmemVariableCache->latestCompletedXid;
> > > +
> >
> > needs a lot more explanation. It took me a very long time to figure out
> > why using latest completed xid is safe.
>
> OK. Took me a long time as well.

Done

> > * Can you merge with CVS HEAD, please? There's some merge conflicts.
>
> Huh? I did. And tested patch against a CVS checkout before submitting.
> Can you explain further?

Still not sure what conflicts you see or where they might come from...

I am now unable to push these changes to the shared repo. What is
happening?

--
Simon Riggs www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I am now unable to push these changes to the shared repo. What is
> happening?

Perhaps you need to run cvs update on your local copy?

(I find this flavor the most useful: "cvs -q update -d" YMMV.)

If that's not it, error message?

...Robert

On Mon, 2009-12-14 at 06:51 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
> >> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
> >> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> >> >> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> >> >> > * Please remove any spurious whitespace. "git diff --color" makes them
> >> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
> >> >> > better to fix them before committing, IMO).
> >> >>
> >> >> +1 in general, not particularly for this patch (haven't checked that
> >> >> in this patch).
> >> >>
> >> >> Actually, how about we add that to the page at
> >> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
> >> >
> >> > If we can define "spurious whitespace" it would help decide whether
> >> > there is any action to take, and when.
> >>
> >> git defines it as either (1) extra whitespace at the end of a line or
> >> (2) an initial indent that uses spaces followed by tabs (typically
> >> something like space-tab, where tab alone would have produced the same
> >> result). git diff --check master tends to be useful here.
> >
> > (2) is a problem that has been discussed before on hackers, anything
> > like that should be changed.
> >
> > Why is (1) important, and if it is important, why is it being mentioned
> > only now? Are we saying that all previous reviewers of my work (and
> > others') removed these without ever mentioning they had done so?
>
> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code. I certainly mention this
> in any review I do where it's applicable, and have been doing so for
> some time. I also will certainly fix it for any code I commit. I
> also mentioned it in the review that I did of Hot Standby.

I don't recall you mentioning that.

There are no changes in this patch that are purely whitespace changes.
Those were removed prior to patch submission. This is all about code I
am adding or changing. My additions may disrupt their patches, but not
because of the whitespace.

If we are going to run pgindent anyway, what is the point? Seems like we
need a tool to fix patches, not a tool to fix the code.

I've made some changes to the larger files.

--
Simon Riggs www.2ndQuadrant.com

On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > I am now unable to push these changes to the shared repo. What is
> > happening?
>
> Perhaps you need to run cvs update on your local copy?
>
> (I find this flavor the most useful: "cvs -q update -d" YMMV.)
>
> If that's not it, error message?

It was a question for Heikki only, not a CVS issue.

git push ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git
hs-simon:hs-riggs
To ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git
! [rejected] hs-simon -> hs-riggs (non-fast forward)
error: failed to push some refs to
'ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git'

--
Simon Riggs www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 13:47, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
>> On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > I am now unable to push these changes to the shared repo. What is
>> > happening?
>>
>> Perhaps you need to run cvs update on your local copy?
>>
>> (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
>>
>> If that's not it, error message?
>
> It was a question for Heikki only, not a CVS issue.
>
> git push ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git
> hs-simon:hs-riggs
> To ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git
>  ! [rejected]        hs-simon -> hs-riggs (non-fast forward)
> error: failed to push some refs to
> 'ssh://git(at)git(dot)postgresql(dot)org/users/heikki/postgres.git'

Same issue can be it in git - did you do a "git pull" before? You may
need merging with what's on there, and for that to work you must pull
before you can push.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> There are no changes in this patch that are purely whitespace changes.
> Those were removed prior to patch submission. This is all about code I
> am adding or changing. My additions may disrupt their patches, but not
> because of the whitespace.
>
> If we are going to run pgindent anyway, what is the point? Seems like we
> need a tool to fix patches, not a tool to fix the code.
>
> I've made some changes to the larger files.

I'll try to explain this again because it is a little bit subtle.

Whenever someone commits ANY patch, there is a danger of disrupting
other outstanding patches that touch the same code. That's basically
unavoidable, although certainly it's a reason to avoid superfluous
changes like whitespace adjustments or useless identifier renaming.

However, there's a second, completely independent issue, which is that
eventually we will run pgindent for 8.5, and when we do that, it's
going to reformat stuff, and that reformatting is going to break
things. The amount of reformatting that it does (and therefore the
amount of breakage that it causes) is directly related to the extent
to which people have committed patches throughout the release cycle
that don't conform to the layout that pgident is going to want. If
every patch perfectly matched the pgident style, then the pgindent run
would change nothing and we would all be VERY happy. The more
deviations there are, the more stuff breaks.

So I agree with you: we need a tool to fix patches. However, since we
haven't got one, we owe it to other contributors not to make the
problem any worse than necessary by adhering to the project's
formatting conventions as best we're able when committing things,
especially with regards to trivial things like trailing white-space
that git diff --check can easily find. Actually, git apply has an
option to fix these simple types of problems, so it's possible to fix
it diffing the patch set against the master branch, applying it to a
separate copy of the master branch using git apply --whitespace=fix,
then diffing that against the original batch and applying the changes.
Although that's usually overkill unless it's really bad.

There's some interesting discussion on whitespace more generally from
Tom Lane here:

http://archives.postgresql.org/pgsql-hackers/2009-08/msg01001.php

...Robert

On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:

> If every patch perfectly matched the pgident style, then the pgindent
> run would change nothing and we would all be VERY happy.

I've made all whitespace changes to the patch.

I understand the reason for acting as you suggest, but we either police
it, or we don't. If we don't police in all cases then I'm not personally
happy to be policed.

About 90% of the whitespace in the patch were in docs, README or test
output files. A great many of that type of file have numerous line
ending whitespace, not introduced by me, so it seems to me that this has
never been adequately policed in the way you say. If we really do care
about this issue then I expect people to look a little further than just
my patch.

Portability of patches across releases isn't a huge issue for me, nor
for the project, I think, but I am willing to commit to cleaning future
patches in this way.

--
Simon Riggs www.2ndQuadrant.com

On Mon, 2009-12-14 at 13:56 +0100, Magnus Hagander wrote:

> Same issue can be it in git - did you do a "git pull" before? You may
> need merging with what's on there, and for that to work you must pull
> before you can push.

Found some merge conflicts and resolved them. I did fetch and merge at
different times, so that seems to be the source.

I've resolved the other git issues, so latest version on git now.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:

> If we are going to run pgindent anyway, what is the point?

Perhaps it would take less time to run this than to argue the point?:

sed -e 's/[ \t][ \t]*$//' -e 's/ *\t/\t/g' *

-Kevin

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Why is (1) important, and if it is important, why is it being mentioned
>> only now? Are we saying that all previous reviewers of my work (and
>> others') removed these without ever mentioning they had done so?

> pgident will remove such white spaces and create merge conflicts for
> everyone working on those areas of the code.

What I try really hard to remove from committed patches is spurious
whitespace changes to pre-existing code. Whether new code blocks
exactly match pgindent's rules is less of a concern, but changing
code you don't have to in a way that pgindent will undo later anyway
is just useless creation of potential conflicts.

The whole thing would be a lot easier if someone would put together an
easily-installable version of pgindent. Bruce has posted the patches he
uses but I don't know what version of indent they're against...

regards, tom lane

On Mon, Dec 14, 2009 at 10:08 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:
>> If every patch perfectly matched the pgident style, then the pgindent
>> run would change nothing and we would all be VERY happy.
>
> I've made all whitespace changes to the patch.
>
> I understand the reason for acting as you suggest, but we either police
> it, or we don't. If we don't police in all cases then I'm not personally
> happy to be policed.

I am doing my best to police it, and certainly will police it for
anything that I review or commit. Heikki was the one who originally
pointed it on this thread, Magnus gave a +1, and I am pretty sure Tom
tries to keep an eye out for it, too, so generally I would say it is
project practice. Obviously I am not able to control the actions of
all the other committers, and it does appear that some crap has crept
in since the last pgindent run. :-(

At any rate I don't think you're being singled out for special treatment.

> About 90% of the whitespace in the patch were in docs, README or test
> output files. A great many of that type of file have numerous line
> ending whitespace, not introduced by me, so it seems to me that this has
> never been adequately policed in the way you say. If we really do care
> about this issue then I expect people to look a little further than just
> my patch.

pgindent won't reindent the docs or the README, but git diff --check
picks up on trailing whitespace, so it may be that Tom (who doesn't
use git but does commit a lot of patches) is less finicky about
trailing whitespace in those places. If we move to git across the
board, I expect this to get more standardized handling, but I think we
have a ways to go on that yet.

> Portability of patches across releases isn't a huge issue for me, nor
> for the project, I think, but I am willing to commit to cleaning future
> patches in this way.

It's a pretty significant issue for me personally, and anyone who is
maintaining a fork of the main source base that has to be merged when
the pgindent run hits. It would be nice if someone wanted to build a
better mousetrap here, but so far no volunteers.

...Robert

Tom Lane escribió:

> The whole thing would be a lot easier if someone would put together an
> easily-installable version of pgindent. Bruce has posted the patches he
> uses but I don't know what version of indent they're against...

And we're still unclear on the typedef list that's going to be used.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
>> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> * Please remove any spurious whitespace. "git diff --color" makes them
>>> stand out like a sore thumb, in red. (pgindent will fix them but always
>>> better to fix them before committing, IMO).
>> +1 in general, not particularly for this patch (haven't checked that
>> in this patch).
>>
>> Actually, how about we add that to the page at
>> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
>
> If we can define "spurious whitespace" it would help decide whether
> there is any action to take, and when.

There's two definitions that are useful:

- Anything that "git diff --color" shows up as glaring red. Important to
remove simply because the red whitespace is distracting when I review a
patch.

- Anything that pgindent would/will eventually fix. Because you might as
well fix them before committing and save the extra churn and potential
(although trivial) merge conflicts in people's outstanding patches when
pgindent is run. I don't run pgindent myself, so I wouldn't notice most
stuff, but I tend to fix things that I do notice.

> Why is (1) important, and if it is important, why is it being mentioned
> only now? Are we saying that all previous reviewers of my work (and
> others') removed these without ever mentioning they had done so?

I did it in one pass, Oct 15th according to git log.

I tend to silently fix whitespace issues like that in all patches I
commit. It's generally trivial enough to fix that it's easier to just
fix it myself than complain, explain, and look like a real nitpick.

I note that if it was easy to run pgindent yourself on a patch before
committing/submitting, we wouldn't need to have this discussion. I don't
know hard it is to get it working right, but I recall I tried once and
gave up. Any volunteers?

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

On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:
>
> > If we are going to run pgindent anyway, what is the point?
>
> Perhaps it would take less time to run this than to argue the point?:
>
> sed -e 's/[ \t][ \t]*$//' -e 's/ *\t/\t/g' *

Not certain that is exactly correct, plus it doesn't only work against a
current patch since there are already many examples of whitespace in CVS
already. I do appreciate your attempts at resolution and an easy
tool-based approach for the future, though I'll let someone else run
with it.

--
Simon Riggs www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 11:09:49AM +0100, Magnus Hagander wrote:
> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> > * Please remove any spurious whitespace.  "git diff --color" makes
> > them stand out like a sore thumb, in red. (pgindent will fix them
> > but always better to fix them before committing, IMO).
>
> +1 in general, not particularly for this patch (haven't checked that
> in this patch).
>
> Actually, how about we add that to the page at
> http://wiki.postgresql.org/wiki/Submitting_a_Patch?

Done.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Dec 14, 2009 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:
>>
>> > If we are going to run pgindent anyway, what is the point?
>>
>> Perhaps it would take less time to run this than to argue the point?:
>>
>> sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
>
> Not certain that is exactly correct, plus it doesn't only work against a
> current patch since there are already many examples of whitespace in CVS
> already. I do appreciate your attempts at resolution and an easy
> tool-based approach for the future, though I'll let someone else run
> with it.

Yeah, that would actually be a disaster, because it would actually add
deltas to the patch in the short term.

Seems to me that we would be better off figuring out which exact ident
Bruce is running and checking the typedef list into CVS.

...Robert

Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> Why is (1) important, and if it is important, why is it being mentioned
> >> only now? Are we saying that all previous reviewers of my work (and
> >> others') removed these without ever mentioning they had done so?
>
> > pgident will remove such white spaces and create merge conflicts for
> > everyone working on those areas of the code.
>
> What I try really hard to remove from committed patches is spurious
> whitespace changes to pre-existing code. Whether new code blocks
> exactly match pgindent's rules is less of a concern, but changing
> code you don't have to in a way that pgindent will undo later anyway
> is just useless creation of potential conflicts.
>
> The whole thing would be a lot easier if someone would put together an
> easily-installable version of pgindent. Bruce has posted the patches he
> uses but I don't know what version of indent they're against...

The entire indent tarball with patches is on our ftp site.

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

+ If your life is a hard drive, Christ can be your backup. +

Heikki Linnakangas wrote:
> I note that if it was easy to run pgindent yourself on a patch before
> committing/submitting, we wouldn't need to have this discussion. I don't
> know hard it is to get it working right, but I recall I tried once and
> gave up.
>
What sort of problems did you run into?

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On Mon, Dec 14, 2009 at 11:07 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
>> on your transaction rate to begin with. Do we really want this setting
>> in its current form? Does it make sense as PGC_USERSET, as if one
>> backend uses a lower setting than others, that's the one that really
>> determines when transactions are killed in the standby? I think this
>> should be discussed and implemented as a separate patch.
>
> All the vacuum_*_age parameters have this characteristic, yet they
> exist.

I think it makes sense to have it be userset because someone could run
vacuum on one table with a different defer_cleanup_age for some
reason. I admit I'm having trouble coming up with a good use case.
Personally I'm fine with having parameters like this in alphas that
end up being renamed, or changed to different semantics, or even
removed by the time it's launched.

>> It doesn't seem any more wrong than using hash indexes right after
>> recovery, crash recovery or otherwise. It's certainly broken, but I
>> don't see much value in a partial fix. The bottom line is that hash
>> indexes should be WAL-logged.
>
> I know that's your thought; I'm just checking its everyone else's as
> well. We go to great lengths elsewhere in the patch to avoid queries
> returning incorrect results and there is a loss of capability as a
> result. I don't want Hash index users to view this as a feature. I don't
> feel too strongly, but it can be argued both ways, at least.

This goes back to your pluggable rmgr point. Someone could add a new
index method and get bogus results on their standby. And unlike hash
indexes where there's some hope of addressing the problem there's
nothing they can do to fix this.

It does seem like having a flag in the catalog to mark nonrecoverable
indexes and make them unavailable to query plans on the standby would
be worth its weight in code.

--
greg

Greg Smith wrote:
> Heikki Linnakangas wrote:
>> I note that if it was easy to run pgindent yourself on a patch before
>> committing/submitting, we wouldn't need to have this discussion. I don't
>> know hard it is to get it working right, but I recall I tried once and
>> gave up.
>>
> What sort of problems did you run into?

I don't remember, unfortunately.

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

Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
>> Simon Riggs wrote:
>> * Are you planning to remove the recovery_connections setting before
>> release? The documentation makes it sound like it's a temporary hack
>> that we're not really sure is needed at all. That's not very comforting.
>
> It has been requested and I agree, so its there. Saying it might be
> removed in future is no more than we do elsewhere and AFAIK we all hope
> it will be. Not sure why that is or isn't comforting.

Now that recovery_connections has a double-role, and does in the master
what the wal_standby_info used to do, the documentation probably should
be clarified that the whole parameter is not going to go away, just the
role in the master.

>> * You removed this comment from KnownAssignedXidsInit:
>>
>> - /*
>> - * XXX: We should check that we don't exceed maxKnownAssignedXids.
>> - * Even though the hash table might hold a few more entries than that,
>> - * we use fixed-size arrays of that size elsewhere and expected all
>> - * entries in the hash table to fit.
>> - */
>>
>> but AFAICS you didn't address the issue. It's referring to the 'xids'
>> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
>> in without checking that it fits.
>
> I have ensured that they are always the same size, by definition, so no
> need to check.

How did you ensure that? The hash table has no hard size limit.

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

On Mon, 2009-12-14 at 18:02 +0000, Greg Stark wrote:
> >> It doesn't seem any more wrong than using hash indexes right after
> >> recovery, crash recovery or otherwise. It's certainly broken, but I
> >> don't see much value in a partial fix. The bottom line is that hash
> >> indexes should be WAL-logged.
> >
> > I know that's your thought; I'm just checking its everyone else's as
> > well. We go to great lengths elsewhere in the patch to avoid queries
> > returning incorrect results and there is a loss of capability as a
> > result. I don't want Hash index users to view this as a feature. I
> don't
> > feel too strongly, but it can be argued both ways, at least.
>
> This goes back to your pluggable rmgr point. Someone could add a new
> index method and get bogus results on their standby. And unlike hash
> indexes where there's some hope of addressing the problem there's
> nothing they can do to fix this.
>
> It does seem like having a flag in the catalog to mark nonrecoverable
> indexes and make them unavailable to query plans on the standby would
> be worth its weight in code.

pg_am.amalmostworks or perhaps pg_am.amhalffinished... :-)

I wouldn't wish to literally persist that situation, especially since if
we had it we couldn't update it during recovery. We need to allow
pluggable indexes in full, not just partially. I think we should extend
pg_am so that rmgr routines can be defined for them also, with dynamic
assignment of rmgrids, recorded in file so we can use them during
recovery.

What we also need is a mechanism to identify and mark indexes as corrupt
while they are being rebuilt, so recovery can complete without them and
then rebuild automatically when recovery finishes. And so they can be
skipped during hot standby.

--
Simon Riggs www.2ndQuadrant.com

On Sun, 2009-12-13 at 22:25 +0000, Simon Riggs wrote:

> * Which exact tables are we talking about: just pg_class and the shared
> catalogs? Everything else is in pg_class, so if we can find it we're OK?
> formrdesc() tells me the list of nailed relations is: pg_database,
> pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
> the ones we care about, or are they just a subset?

Comments in cluster.c's check_index_is_clusterable() suggest that the
list of tables to which this applies is nailed relations *and* shared
relations, plus their indexes.

/*
* Disallow clustering system relations. This will definitely NOT work
* for shared relations (we have no way to update pg_class rows in other
* databases), nor for nailed-in-cache relations (the relfilenode values
* for those are hardwired, see relcache.c). It might work for other
* system relations, but I ain't gonna risk it.
*/

So that means we need to handle 3 cases: nailed-local, nailed-shared and
non-nailed-shared.

I would presume we would not want to relax the restriction on CLUSTER
working on these tables, even if new VACUUM FULL does.

Anyway, not going to be done for Alpha3, but seems fairly doable now.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> * Disallow clustering system relations. This will definitely NOT work
> * for shared relations (we have no way to update pg_class rows in other
> * databases), nor for nailed-in-cache relations (the relfilenode values
> * for those are hardwired, see relcache.c). It might work for other
> * system relations, but I ain't gonna risk it.

> I would presume we would not want to relax the restriction on CLUSTER
> working on these tables, even if new VACUUM FULL does.

Why not? If we solve the problem of allowing these relations to change
relfilenodes, then CLUSTER would work just fine on them. Whether it's
particularly useful is not ours to decide I think.

regards, tom lane

On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:

> >> * You removed this comment from KnownAssignedXidsInit:
> >>
> >> - /*
> >> - * XXX: We should check that we don't exceed maxKnownAssignedXids.
> >> - * Even though the hash table might hold a few more entries than that,
> >> - * we use fixed-size arrays of that size elsewhere and expected all
> >> - * entries in the hash table to fit.
> >> - */
> >>
> >> but AFAICS you didn't address the issue. It's referring to the 'xids'
> >> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
> >> in without checking that it fits.
> >
> > I have ensured that they are always the same size, by definition, so no
> > need to check.
>
> How did you ensure that? The hash table has no hard size limit.

The hash table is in shared memory and the entry size is fixed. My
understanding was that this meant the hash table was fixed in size and
could not grow beyond the allocation. If that assumption was wrong, then
yes we could get an error. Is it? Do you know from experience, or from
code comments?

Incidentally just picked up two much easier issues in that stretch of
code. Thanks for making me look again!

--
Simon Riggs www.2ndQuadrant.com

On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > * Disallow clustering system relations. This will definitely NOT work
> > * for shared relations (we have no way to update pg_class rows in other
> > * databases), nor for nailed-in-cache relations (the relfilenode values
> > * for those are hardwired, see relcache.c). It might work for other
> > * system relations, but I ain't gonna risk it.
>
> > I would presume we would not want to relax the restriction on CLUSTER
> > working on these tables, even if new VACUUM FULL does.
>
> Why not? If we solve the problem of allowing these relations to change
> relfilenodes, then CLUSTER would work just fine on them. Whether it's
> particularly useful is not ours to decide I think.

I think you are probably right, but my wish to prove Schrodinger's Bug
does not exist is not high enough for me personally to open that box
this side of 8.6, especially when the previous code author saw it as a
risk worth documenting.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>>> * Disallow clustering system relations. This will definitely NOT work
>>> * for shared relations (we have no way to update pg_class rows in other
>>> * databases), nor for nailed-in-cache relations (the relfilenode values
>>> * for those are hardwired, see relcache.c). It might work for other
>>> * system relations, but I ain't gonna risk it.
>>
>>> I would presume we would not want to relax the restriction on CLUSTER
>>> working on these tables, even if new VACUUM FULL does.
>>
>> Why not? If we solve the problem of allowing these relations to change
>> relfilenodes, then CLUSTER would work just fine on them. Whether it's
>> particularly useful is not ours to decide I think.

> I think you are probably right, but my wish to prove Schrodinger's Bug
> does not exist is not high enough for me personally to open that box
> this side of 8.6, especially when the previous code author saw it as a
> risk worth documenting.

You're talking to the "previous code author" ... or at least I'm pretty
sure that comment is mine.

regards, tom lane

On Mon, 2009-12-14 at 14:14 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
> >> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> >>> * Disallow clustering system relations. This will definitely NOT work
> >>> * for shared relations (we have no way to update pg_class rows in other
> >>> * databases), nor for nailed-in-cache relations (the relfilenode values
> >>> * for those are hardwired, see relcache.c). It might work for other
> >>> * system relations, but I ain't gonna risk it.
> >>
> >>> I would presume we would not want to relax the restriction on CLUSTER
> >>> working on these tables, even if new VACUUM FULL does.
> >>
> >> Why not? If we solve the problem of allowing these relations to change
> >> relfilenodes, then CLUSTER would work just fine on them. Whether it's
> >> particularly useful is not ours to decide I think.
>
> > I think you are probably right, but my wish to prove Schrodinger's Bug
> > does not exist is not high enough for me personally to open that box
> > this side of 8.6, especially when the previous code author saw it as a
> > risk worth documenting.
>
> You're talking to the "previous code author" ... or at least I'm pretty
> sure that comment is mine.

Yeh, I figured, but I'm just as scared now as you were back then.

This might allow CLUSTER to work, but it is definitely not something
that I will enabling, testing and committing to fix *when* it breaks
because my time is already allocated on other stuff.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
>>> I have ensured that they are always the same size, by definition, so no
>>> need to check.
>>
>> How did you ensure that? The hash table has no hard size limit.

> The hash table is in shared memory and the entry size is fixed. My
> understanding was that this meant the hash table was fixed in size and
> could not grow beyond the allocation. If that assumption was wrong, then
> yes we could get an error. Is it?

Entirely. The only thing the hash table size enters into is the sizing
of overall shared memory --- different hash tables then consume space
from the common pool, which includes not only the computed space
requirements but a pretty hefty slop overhead. You can go beyond the
original requested space if there is any slop left.

For a number of shared hashtables that actually have a fixed set of
entries, we avoid the risk of unexpected out-of-memory by forcing all
the entries to come into existence during startup. If your table
doesn't work that way then you cannot be sure of the exact point where
it will get an out-of-memory failure.

regards, tom lane

On Mon, 2009-12-14 at 15:24 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
> >>> I have ensured that they are always the same size, by definition, so no
> >>> need to check.
> >>
> >> How did you ensure that? The hash table has no hard size limit.
>
> > The hash table is in shared memory and the entry size is fixed. My
> > understanding was that this meant the hash table was fixed in size and
> > could not grow beyond the allocation. If that assumption was wrong, then
> > yes we could get an error. Is it?
>
> Entirely. The only thing the hash table size enters into is the sizing
> of overall shared memory --- different hash tables then consume space
> from the common pool, which includes not only the computed space
> requirements but a pretty hefty slop overhead. You can go beyond the
> original requested space if there is any slop left.

OK, thanks.

> For a number of shared hashtables that actually have a fixed set of
> entries, we avoid the risk of unexpected out-of-memory by forcing all
> the entries to come into existence during startup. If your table
> doesn't work that way then you cannot be sure of the exact point where
> it will get an out-of-memory failure.

The data structure was originally a list of fixed size, though is now a
shared hash table.

What is the best way of restricting the hash table to a maximum size?

Your last para makes me think there is a way, but I can't see it
directly. If there isn't a facility to do this and I need to add code,
should I add optional code into the dynahash.c to track size, or should
I add that in the data structure code that uses the hash functions (so,
internally or externally).

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> What is the best way of restricting the hash table to a maximum size?

There is nothing in dynahash that will enforce a maximum size against
calling code that's not cooperating; and I'll resist any attempt to
add such a thing, because it would create a serialization point across
the whole hashtable.

If you know that you need at most N entries in the hash table, you can
preallocate that many at startup (note the second arg to ShmemInitHash)
and be safe. If your calling code might go past that, you'll need to
fix the calling code.

regards, tom lane

On Mon, 2009-12-14 at 16:39 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > What is the best way of restricting the hash table to a maximum size?
>
> There is nothing in dynahash that will enforce a maximum size against
> calling code that's not cooperating; and I'll resist any attempt to
> add such a thing, because it would create a serialization point across
> the whole hashtable.

No problem, just checking with you where you'd like stuff put.

> If you know that you need at most N entries in the hash table, you can
> preallocate that many at startup (note the second arg to ShmemInitHash)
> and be safe. If your calling code might go past that, you'll need to
> fix the calling code.

It's easy enough to count em on the way in and count em on the way out.

--
Simon Riggs www.2ndQuadrant.com

On Mon, 2009-12-14 at 11:44 +0200, Peter Eisentraut wrote:
> On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
> > Wednesday because that seemed a good delay to allow review. Josh and I
> > had discussed the value of getting patch into Alpha3, so that was my
> > wish and aim.
> >
> > I'm not aware of any particular date for end of commitfest, though
> > possibly you are about to update me on that?
>
> Commit fests for 8.5 have usually run from the 15th to the 15th of next
> month, but the CF manager may choose to vary that.
>
> FWIW, the alpha release manager may also vary the release timeline of
> alpha3. ;-)

I'm hoping that the alpha release manager can wait until Wednesday,
please.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> On Mon, 2009-12-14 at 11:44 +0200, Peter Eisentraut wrote:
>
>> On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
>>
>>> Wednesday because that seemed a good delay to allow review. Josh and I
>>> had discussed the value of getting patch into Alpha3, so that was my
>>> wish and aim.
>>>
>>> I'm not aware of any particular date for end of commitfest, though
>>> possibly you are about to update me on that?
>>>
>> Commit fests for 8.5 have usually run from the 15th to the 15th of next
>> month, but the CF manager may choose to vary that.
>>
>> FWIW, the alpha release manager may also vary the release timeline of
>> alpha3. ;-)
>>
>
> I'm hoping that the alpha release manager can wait until Wednesday,
> please.
>
At this point we've got 5 small to medium sized patches in the "Ready
for Committer" queue. Maybe that gets all done on Tuesday, maybe it
slips to Wednesday or later. It's not like a bell goes off tomorrow and
we're done; there's probably going to be just a little slip here.

In any case, it's certainly not the case that this is all done right now
such that the alpha gets packed on Tuesday just because it's the 15th.
It sounds like the worst case is that alpha would have to wait a day for
Hot Standby to be finally committed, which seems well worth doing if it
means HS gets that much more testing on it. It would be a help to
eliminate the merge conflict issues for the Streaming Replication team
by giving them only one code base to worry about merges against. And on
the PR side, announcing HS as hitting core and available in the alpha is
huge.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
> Barring resolving a few points and subject to even more testing, this
> is the version I expect to commit to CVS on Wednesday.

So it's Thursday now. Please keep us updated on the schedule, as we
need to decide when to wrap alpha3 and whether to reopen the floodgates
for post-CF commits.

On Thu, 2009-12-17 at 12:01 +0200, Peter Eisentraut wrote:
> On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
> > Barring resolving a few points and subject to even more testing, this
> > is the version I expect to commit to CVS on Wednesday.
>
> So it's Thursday now. Please keep us updated on the schedule, as we
> need to decide when to wrap alpha3 and whether to reopen the floodgates
> for post-CF commits.

I've been looking at a semaphore deadlock problem reported by Hiroyuki
Yamada. It's a serious issue, though luckily somewhat restricted in
scope.

I don't think its wise to rush in with a solution, since that involves
some work with semaphores and I could easily make that area less stable
by acting too quickly.

What I will do now is put in a restriction on lock waits in Hot Standby,
which will only happen for Alpha3. That avoids the deadlock issue in an
easy and safe way, though it is heavy handed and I aim to replace it
fairly soon, following discussion and testing.

--
Simon Riggs www.2ndQuadrant.com