Thread: 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@git.postgresql.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
Attachment
Simon Riggs <simon@2ndQuadrant.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@2ndQuadrant.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.linnakangas@enterprisedb.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 HaganderMe: 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.linnakangas@enterprisedb.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@2ndquadrant.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@2ndquadrant.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.linnakangas@enterprisedb.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@2ndquadrant.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.linnakangas@enterprisedb.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@2ndquadrant.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@2ndquadrant.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.linnakangas@enterprisedb.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@gmail.com> wrote: > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.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@2ndquadrant.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.linnakangas@enterprisedb.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 HaganderMe: 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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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.linnakangas@enterprisedb.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@2ndquadrant.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@git.postgresql.org/users/heikki/postgres.git hs-simon:hs-riggs To ssh://git@git.postgresql.org/users/heikki/postgres.git! [rejected] hs-simon -> hs-riggs (non-fast forward) error: failed to push some refs to 'ssh://git@git.postgresql.org/users/heikki/postgres.git' -- Simon Riggs www.2ndQuadrant.com
On Mon, Dec 14, 2009 at 13:47, Simon Riggs <simon@2ndquadrant.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@2ndquadrant.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@git.postgresql.org/users/heikki/postgres.git > hs-simon:hs-riggs > To ssh://git@git.postgresql.org/users/heikki/postgres.git > ! [rejected] hs-simon -> hs-riggs (non-fast forward) > error: failed to push some refs to > 'ssh://git@git.postgresql.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs <simon@2ndquadrant.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 reallybad. 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
Simon Riggs <simon@2ndQuadrant.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
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
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.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@2ndquadrant.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.linnakangas@enterprisedb.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@2ndQuadrant.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.linnakangas@enterprisedb.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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.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@2ndquadrant.com> wrote: > On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote: >> Simon Riggs <simon@2ndQuadrant.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@gmail.com> writes: > > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.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@momjian.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@2ndQuadrant.com www.2ndQuadrant.com
On Mon, Dec 14, 2009 at 11:07 AM, Simon Riggs <simon@2ndquadrant.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 updatepg_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@2ndQuadrant.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@2ndQuadrant.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@2ndQuadrant.com> writes:
> On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
>> Simon Riggs <simon@2ndQuadrant.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@2ndQuadrant.com> writes: > > On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote: > >> Simon Riggs <simon@2ndQuadrant.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@2ndQuadrant.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@2ndQuadrant.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@2ndQuadrant.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@2ndQuadrant.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: <blockquote cite="mid:1260835410.1955.2845.camel@ebony" type="cite"><pre wrap="">On Mon, 2009-12-14 at11:44 +0200, Peter Eisentraut wrote: </pre><blockquote type="cite"><pre wrap="">On mån, 2009-12-14 at 08:54 +0000, SimonRiggs wrote: </pre><blockquote type="cite"><pre wrap="">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? </pre></blockquote><pre wrap="">Commit fests for 8.5 have usually run fromthe 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. ;-) </pre></blockquote><pre wrap=""> I'm hoping that the alpha release manager can wait until Wednesday, please. </pre></blockquote> 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 tomorrowand we're done; there's probably going to be just a little slip here.<br /><br /> In any case, it's certainly notthe case that this is all done right now such that the alpha gets packed on Tuesday just because it's the 15th. It soundslike the worst case is that alpha would have to wait a day for Hot Standby to be finally committed, which seems wellworth doing if it means HS gets that much more testing on it. It would be a help to eliminate the merge conflict issuesfor the Streaming Replication team by giving them only one code base to worry about merges against. And on the PRside, announcing HS as hitting core and available in the alpha is huge.<br /><br /><pre class="moz-signature" cols="72">-- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support <a class="moz-txt-link-abbreviated" href="mailto:greg@2ndQuadrant.com">greg@2ndQuadrant.com</a> <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.com">www.2ndQuadrant.com</a> </pre>
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