Thread: VACUUM (DISABLE_PAGE_SKIPPING on)
The docs are misleading for this feature, since they say: "This option disables all page-skipping behavior, and is intended to be used only when the contents of the visibility map are suspect, which should happen only if there is a hardware or software issue causing database corruption." The docs do correctly say "Pages where all tuples are known to be frozen can always be skipped". Checking the code, lazy_scan_heap() comments say "we can still skip pages that are all-frozen, since such pages do not need freezing". The code is quite clear: DISABLE_PAGE_SKIPPING makes the vacuum into an aggressive vacuum. Line 487, heap_vacuum_rel(). Aggressive vacuums can still skip a page that is frozen, and rely on the visibility map for that information. So the docs are wrong - we don't disable *all* page-skipping and it is not appropriate to warn users away from this feature by saying "is intended to be used only when the contents of the visibility map are suspect". Reworded docs patch attached. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Mon, Nov 16, 2020 at 1:52 PM Simon Riggs <simon@2ndquadrant.com> wrote:
The docs are misleading for this feature, since they say:
"This option disables all page-skipping behavior, and is
intended to be used only when the contents of the visibility map are
suspect, which should happen only if there is a hardware or software
issue causing database corruption."
[...]
The code is quite clear: DISABLE_PAGE_SKIPPING makes the vacuum into
an aggressive vacuum. Line 487, heap_vacuum_rel(). Aggressive vacuums
can still skip a page that is frozen, and rely on the visibility map
for that information.
So the docs are wrong - we don't disable *all* page-skipping and it is
not appropriate to warn users away from this feature by saying "is
intended to be used only when the contents of the visibility map are
suspect".
This patch seems mis-placed, at least in HEAD. If DISABLE_PAGE_SKIPPING isn't doing what the documentation says it should, but instead provides identical behavior to FREEZE, then the bug should be fixed in HEAD. I'd argue for batch-patching it as well.
David J.
On Tue, Nov 17, 2020 at 5:52 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > The docs are misleading for this feature, since they say: > "This option disables all page-skipping behavior, and is > intended to be used only when the contents of the visibility map are > suspect, which should happen only if there is a hardware or software > issue causing database corruption." > > The docs do correctly say "Pages where all tuples are known to be > frozen can always be skipped". Checking the code, lazy_scan_heap() > comments say > "we can still skip pages that are all-frozen, since such pages do not > need freezing". > > The code is quite clear: DISABLE_PAGE_SKIPPING makes the vacuum into > an aggressive vacuum. Line 487, heap_vacuum_rel(). Aggressive vacuums > can still skip a page that is frozen, and rely on the visibility map > for that information. > > So the docs are wrong - we don't disable *all* page-skipping and it is > not appropriate to warn users away from this feature by saying "is > intended to be used only when the contents of the visibility map are > suspect". I don't think the doc is wrong. If DISABLE_PAGE_SKIPPING is specified, we not only set aggressive = true but also skip checking visibility map. For instance, see line 905 and line 963, lazy_scan_heap(). Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Mon, 16 Nov 2020 at 22:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Tue, Nov 17, 2020 at 5:52 AM Simon Riggs <simon@2ndquadrant.com> wrote: > I don't think the doc is wrong. If DISABLE_PAGE_SKIPPING is specified, > we not only set aggressive = true but also skip checking visibility > map. For instance, see line 905 and line 963, lazy_scan_heap(). OK, so you're saying that the docs illustrate the true intention of the patch, which I immediately accept since I know you were the author. Forgive me for not discussing it with you first, I thought this was a clear cut case. But that then highlights another area where the docs are wrong... > On Tue, Nov 17, 2020 at 5:52 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > The docs do correctly say "Pages where all tuples are known to be > > frozen can always be skipped". Checking the code, lazy_scan_heap() > > comments say > > "we can still skip pages that are all-frozen, since such pages do not > > need freezing". The docs say this: "Pages where all tuples are known to be frozen can always be skipped." Why bother to say that if the feature then ignores that point and scans them anyway? May I submit a patch to remove that sentence? Anyway, we're back to where I started: looking for a user-initiated command option that allows a table to scanned aggressively so that relfrozenxid can be moved forward as quickly as possible. This is what I thought that you were trying to achieve with DISABLE_PAGE_SKIPPING option, my bad. Now David J, above, says this would be VACUUM FREEZE, but I don't think that is right. Setting VACUUM FREEZE has these effects: 1) makes a vacuum aggressive, but it also 2) moves the freeze limit so high that it freezes mostly everything. (1) allows the vacuum to reset relfrozenxid, but (2) actually slows down the scan by making it freeze more blocks than it would do normally. So we have 3 ways to reset relfrozenxid by a user action: VACUUM (DISABLE_PAGE_SKIPPING ON) - scans all blocks, deliberately ignoring the ones it could have skipped. This certainly slows it down. VACUUM (FREEZE ON) - freezes everything in its path, slowing down the scan by writing too many blocks. VACUUM (FULL on) - rewrites table and rebuilds index, so very slow What I think we need is a 4th option which aims to move relfrozenxid forwards as quickly as possible * initiates an aggressive scan, so it does not skip blocks because of busy buffer pins * skip pages that are all-frozen, as we are allowed to do * uses normal freeze limits, so we avoid writing to blocks if possible If we do all 3 of those things, the scan will complete as quickly as possible and reset relfrozenxid quickly. It would make sense to use that in conjunction with index_cleanup=off As an additional optimization, if we do find a row that needs freezing on a data block, we should simply freeze *all* row versions on the page, not just the ones below the selected cutoff. This is justified since writing the block is the biggest cost and it doesn't make much sense to leave a few rows unfrozen on a block that we are dirtying. Thoughts? -- Simon Riggs http://www.EnterpriseDB.com/
On 2020-Nov-17, Simon Riggs wrote: > As an additional optimization, if we do find a row that needs freezing > on a data block, we should simply freeze *all* row versions on the > page, not just the ones below the selected cutoff. This is justified > since writing the block is the biggest cost and it doesn't make much > sense to leave a few rows unfrozen on a block that we are dirtying. Yeah. We've had ealier proposals to use high and low watermarks: if any tuple is past the high watermark, then freeze all tuples that are past the low watermark. However this is ancient thinking (prior to HEAP_XMIN_FROZEN) and we don't need the low watermark to be different from zero, since the original xid is retained anyway. So +1 for this idea.
On Tue, Nov 17, 2020 at 8:27 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Mon, 16 Nov 2020 at 22:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Tue, Nov 17, 2020 at 5:52 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > > I don't think the doc is wrong. If DISABLE_PAGE_SKIPPING is specified, > > we not only set aggressive = true but also skip checking visibility > > map. For instance, see line 905 and line 963, lazy_scan_heap(). > > OK, so you're saying that the docs illustrate the true intention of > the patch, which I immediately accept since I know you were the > author. Forgive me for not discussing it with you first, I thought > this was a clear cut case. > > But that then highlights another area where the docs are wrong... > > > On Tue, Nov 17, 2020 at 5:52 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > > The docs do correctly say "Pages where all tuples are known to be > > > frozen can always be skipped". Checking the code, lazy_scan_heap() > > > comments say > > > "we can still skip pages that are all-frozen, since such pages do not > > > need freezing". > > The docs say this: > "Pages where all tuples are known to be frozen can always be skipped." > Why bother to say that if the feature then ignores that point and > scans them anyway? > May I submit a patch to remove that sentence? I think the docs describe the page skipping behaviour first and then describe what DISABLE_PAGE_SKIPPING option does. When discussing this feature, I thought this sentence would help users to understand that because users might be confused with that option name without description of page skipping. So I'm not sure it's unnecessary but if this has confused you, it would need to be somehow improved. > > Anyway, we're back to where I started: looking for a user-initiated > command option that allows a table to scanned aggressively so that > relfrozenxid can be moved forward as quickly as possible. This is what > I thought that you were trying to achieve with DISABLE_PAGE_SKIPPING > option, my bad. > > Now David J, above, says this would be VACUUM FREEZE, but I don't > think that is right. Setting VACUUM FREEZE has these effects: 1) makes > a vacuum aggressive, but it also 2) moves the freeze limit so high > that it freezes mostly everything. (1) allows the vacuum to reset > relfrozenxid, but (2) actually slows down the scan by making it freeze > more blocks than it would do normally. > > So we have 3 ways to reset relfrozenxid by a user action: > VACUUM (DISABLE_PAGE_SKIPPING ON) - scans all blocks, deliberately > ignoring the ones it could have skipped. This certainly slows it down. > VACUUM (FREEZE ON) - freezes everything in its path, slowing down the > scan by writing too many blocks. > VACUUM (FULL on) - rewrites table and rebuilds index, so very slow > > What I think we need is a 4th option which aims to move relfrozenxid > forwards as quickly as possible > * initiates an aggressive scan, so it does not skip blocks because of > busy buffer pins > * skip pages that are all-frozen, as we are allowed to do > * uses normal freeze limits, so we avoid writing to blocks if possible > This can be done with VACUUM today by vacuum_freeze_table_age and vacuum_multixact_freeze_table_age to 0. Adding an option for this behavior would be good for users to understand and would work well with the optimization. > If we do all 3 of those things, the scan will complete as quickly as > possible and reset relfrozenxid quickly. It would make sense to use > that in conjunction with index_cleanup=off Agreed. > > As an additional optimization, if we do find a row that needs freezing > on a data block, we should simply freeze *all* row versions on the > page, not just the ones below the selected cutoff. This is justified > since writing the block is the biggest cost and it doesn't make much > sense to leave a few rows unfrozen on a block that we are dirtying. +1 Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Wed, 18 Nov 2020 at 10:28, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > So we have 3 ways to reset relfrozenxid by a user action: > > VACUUM (DISABLE_PAGE_SKIPPING ON) - scans all blocks, deliberately > > ignoring the ones it could have skipped. This certainly slows it down. > > VACUUM (FREEZE ON) - freezes everything in its path, slowing down the > > scan by writing too many blocks. > > VACUUM (FULL on) - rewrites table and rebuilds index, so very slow > > > > What I think we need is a 4th option which aims to move relfrozenxid > > forwards as quickly as possible > > * initiates an aggressive scan, so it does not skip blocks because of > > busy buffer pins > > * skip pages that are all-frozen, as we are allowed to do > > * uses normal freeze limits, so we avoid writing to blocks if possible > > > > This can be done with VACUUM today by vacuum_freeze_table_age and > vacuum_multixact_freeze_table_age to 0. Adding an option for this > behavior would be good for users to understand and would work well > with the optimization. > > > If we do all 3 of those things, the scan will complete as quickly as > > possible and reset relfrozenxid quickly. It would make sense to use > > that in conjunction with index_cleanup=off > > Agreed. Patches attached. 1. vacuum_anti_wraparound.v2.patch 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > As an additional optimization, if we do find a row that needs freezing > > on a data block, we should simply freeze *all* row versions on the > > page, not just the ones below the selected cutoff. This is justified > > since writing the block is the biggest cost and it doesn't make much > > sense to leave a few rows unfrozen on a block that we are dirtying. > > +1 I'll work on that. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > Patches attached. > 1. vacuum_anti_wraparound.v2.patch > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) I don't like the use of ANTI_WRAPAROUND as a name for this new option. Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something else, but I dislike anti-wraparound. It's neither the most aggressive thing we can do to prevent wraparound (that's FREEZE), nor is it the case that vacuums without this option (or indeed any options) can't help prevent wraparound, because the aggressive strategy may be chosen anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > Patches attached. > > 1. vacuum_anti_wraparound.v2.patch > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > I don't like the use of ANTI_WRAPAROUND as a name for this new option. > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something > else, but I dislike anti-wraparound. -1 for using the term AGGRESSIVE, which seems likely to offend people. I'm sure a more descriptive term exists. > It's neither the most aggressive > thing we can do to prevent wraparound (that's FREEZE), The new option is not the same thing as the FREEZE option, as discussed above. > nor is it the > case that vacuums without this option (or indeed any options) can't > help prevent wraparound, because the aggressive strategy may be > chosen anyway. Maybe. The "aim [is] to move relfrozenxid forwards as quickly as possible" so as to avoid wraparound, so having an unambiguous command that does that is important for usability. It also allows us to rely on the user's explicit intention to optimize vacuum towards that goal. -- Simon Riggs http://www.EnterpriseDB.com/
On Wed, 18 Nov 2020 at 02:04, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2020-Nov-17, Simon Riggs wrote: > > > As an additional optimization, if we do find a row that needs freezing > > on a data block, we should simply freeze *all* row versions on the > > page, not just the ones below the selected cutoff. This is justified > > since writing the block is the biggest cost and it doesn't make much > > sense to leave a few rows unfrozen on a block that we are dirtying. > > Yeah. We've had earlier proposals to use high and low watermarks: if any > tuple is past the high watermark, then freeze all tuples that are past > the low watermark. However this is ancient thinking (prior to > HEAP_XMIN_FROZEN) and we don't need the low watermark to be different > from zero, since the original xid is retained anyway. > > So +1 for this idea. Patch to do this attached, for discussion. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > Patches attached. > > > 1. vacuum_anti_wraparound.v2.patch > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option. > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something > > else, but I dislike anti-wraparound. > > -1 for using the term AGGRESSIVE, which seems likely to offend people. > I'm sure a more descriptive term exists. Since we use the term aggressive scan in the docs, I personally don't feel unnatural about that. But since this option also disables index cleanup when not enabled explicitly, I’m concerned a bit if user might get confused. I came up with some names like FEEZE_FAST and FREEZE_MINIMAL but I'm not sure these are better. BTW if this option also disables index cleanup for faster freezing, why don't we disable heap truncation as well? Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Fri, Nov 20, 2020 at 6:02 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Wed, 18 Nov 2020 at 02:04, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2020-Nov-17, Simon Riggs wrote: > > > > > As an additional optimization, if we do find a row that needs freezing > > > on a data block, we should simply freeze *all* row versions on the > > > page, not just the ones below the selected cutoff. This is justified > > > since writing the block is the biggest cost and it doesn't make much > > > sense to leave a few rows unfrozen on a block that we are dirtying. > > > > Yeah. We've had earlier proposals to use high and low watermarks: if any > > tuple is past the high watermark, then freeze all tuples that are past > > the low watermark. However this is ancient thinking (prior to > > HEAP_XMIN_FROZEN) and we don't need the low watermark to be different > > from zero, since the original xid is retained anyway. > > > > So +1 for this idea. > > Patch to do this attached, for discussion. Thank you for the patch! + * + * Once we decide to dirty the data block we may as well freeze + * any tuples that are visible to all, since the additional + * cost of freezing multiple tuples is low. I'm concerned that always freezing all tuples when we're going to make the page dirty would affect the existing vacuum workload much. The additional cost of freezing multiple tuples would be low but if we freeze tuples we would also need to write WAL, which is not negligible overhead I guess. In the worst case, if a table has dead tuples on all pages we process them, but with this patch, in addition to that, we will end up not only freezing all live tuples but also writing XLOG_HEAP2_FREEZE_PAGE WAL for all pages. So I think it would be better either to freeze all tuples if we find a tuple that needs to be frozen or to make this behavior work only if the new VACUUM option is specified. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > Patches attached. > > > > 1. vacuum_anti_wraparound.v2.patch > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option. > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something > > > else, but I dislike anti-wraparound. > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people. > > I'm sure a more descriptive term exists. > > Since we use the term aggressive scan in the docs, I personally don't > feel unnatural about that. But since this option also disables index > cleanup when not enabled explicitly, I’m concerned a bit if user might > get confused. I came up with some names like FEEZE_FAST and > FREEZE_MINIMAL but I'm not sure these are better. FREEZE_FAST seems good. > BTW if this option also disables index cleanup for faster freezing, > why don't we disable heap truncation as well? Good idea -- Simon Riggs http://www.2ndQuadrant.com/
On Fri, 20 Nov 2020 at 03:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > So +1 for this idea. > > > > Patch to do this attached, for discussion. > > Thank you for the patch! > > + * > + * Once we decide to dirty the data block we may as well freeze > + * any tuples that are visible to all, since the additional > + * cost of freezing multiple tuples is low. > > I'm concerned that always freezing all tuples when we're going to make > the page dirty would affect the existing vacuum workload much. The > additional cost of freezing multiple tuples would be low but if we > freeze tuples we would also need to write WAL, which is not negligible > overhead I guess. In the worst case, if a table has dead tuples on all > pages we process them, but with this patch, in addition to that, we > will end up not only freezing all live tuples but also writing > XLOG_HEAP2_FREEZE_PAGE WAL for all pages. So I think it would be > better either to freeze all tuples if we find a tuple that needs to be > frozen or to make this behavior work only if the new VACUUM option is > specified. The additional cost of freezing is sizeof(xl_heap_freeze_tuple) = 11 bytes I guess there is some overhead for writing the WAL record itself, the only question is how much. If that is a concern then we definitely don't want to do that only when using FAST_FREEZE, since that would slow it down when we want to speed it up. I've updated the patch to match your proposal, so we can compare. It seems a shorter patch. (This patch is an optimization that is totally independent to the other proposals on this thread). -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote: > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > > Patches attached. > > > > > 1. vacuum_anti_wraparound.v2.patch > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > > > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option. > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something > > > > else, but I dislike anti-wraparound. > > > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people. > > > I'm sure a more descriptive term exists. > > > > Since we use the term aggressive scan in the docs, I personally don't > > feel unnatural about that. But since this option also disables index > > cleanup when not enabled explicitly, I’m concerned a bit if user might > > get confused. I came up with some names like FEEZE_FAST and > > FREEZE_MINIMAL but I'm not sure these are better. > > FREEZE_FAST seems good. > > > BTW if this option also disables index cleanup for faster freezing, > > why don't we disable heap truncation as well? > > Good idea Patch attached, using the name "FAST_FREEZE" instead. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On 2020-Nov-20, Masahiko Sawada wrote: > I'm concerned that always freezing all tuples when we're going to make > the page dirty would affect the existing vacuum workload much. The > additional cost of freezing multiple tuples would be low but if we > freeze tuples we would also need to write WAL, which is not negligible > overhead I guess. In the worst case, if a table has dead tuples on all > pages we process them, but with this patch, in addition to that, we > will end up not only freezing all live tuples but also writing > XLOG_HEAP2_FREEZE_PAGE WAL for all pages. So I think it would be > better either to freeze all tuples if we find a tuple that needs to be > frozen or to make this behavior work only if the new VACUUM option is > specified. There are two costs associated with this processing. One is dirtying the page (which means it needs to be written down when evicted), and the other is to write WAL records for each change. The cost for the latter is going to be the same in both cases (with this change and without) because the same WAL will have to be written -- the only difference is *when* do you pay it. The cost of the former is quite different; with Simon's patch we dirty the page once, and without the patch we may dirty it several times before it becomes "stable" and no more writes are done to it. (If you have tables whose pages change all the time, there would be no difference with or without the patch.) Dirtying the page less times means less full-page images to WAL, too, which can be significant.
On Fri, 20 Nov 2020 at 14:07, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2020-Nov-20, Masahiko Sawada wrote: > > > I'm concerned that always freezing all tuples when we're going to make > > the page dirty would affect the existing vacuum workload much. The > > additional cost of freezing multiple tuples would be low but if we > > freeze tuples we would also need to write WAL, which is not negligible > > overhead I guess. In the worst case, if a table has dead tuples on all > > pages we process them, but with this patch, in addition to that, we > > will end up not only freezing all live tuples but also writing > > XLOG_HEAP2_FREEZE_PAGE WAL for all pages. So I think it would be > > better either to freeze all tuples if we find a tuple that needs to be > > frozen or to make this behavior work only if the new VACUUM option is > > specified. > > There are two costs associated with this processing. One is dirtying > the page (which means it needs to be written down when evicted), and the > other is to write WAL records for each change. The cost for the latter > is going to be the same in both cases (with this change and without) > because the same WAL will have to be written -- the only difference is > *when* do you pay it. The cost of the former is quite different; with > Simon's patch we dirty the page once, and without the patch we may dirty > it several times before it becomes "stable" and no more writes are done > to it. > > (If you have tables whose pages change all the time, there would be no > difference with or without the patch.) > > Dirtying the page less times means less full-page images to WAL, too, > which can be significant. Summary of patch effects: one_freeze_then_max_freeze.v5.patch doesn't increase/decrease the number of pages that are dirtied by freezing, but when a page will be dirtied **by any activity** it maximises the number of rows frozen, so in some cases it may write a WAL freeze record when it would not have done previously. one_freeze_then_max_freeze.v6.patch doesn't increase/decrease the number of pages that are dirtied by freezing, but when a page will be dirtied **by freezing only** it maximises the number of rows frozen, so the number of WAL records for freezing is the same, but they may contain more tuples than before and will increase the probability that the page is marked all_frozen. So yes, as you say, the net effect will be to reduce the number of write I/Os in subsequent vacuums required to move forward relfrozenxid. -- Simon Riggs http://www.EnterpriseDB.com/
Note on heap_prepare_freeze_tuple()'s fifth parameter, it's not valid to pass OldestXmin; you need a multixact limit there, not an Xid limit. I think the return value of GetOldestMultiXactId is a good choice. AFAICS this means that you'll need to add a new output argument to vacuum_set_xid_limits (You *could* call GetOldestMultiXactId again, but it seems a waste). Maybe it's time for vacuum_set_xid_limits to have a caller's-stack- allocated struct for input and output values, rather than so many arguments and output pointers to fill.
On 2020-Nov-20, Simon Riggs wrote: > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Since we use the term aggressive scan in the docs, I personally don't > > feel unnatural about that. But since this option also disables index > > cleanup when not enabled explicitly, I’m concerned a bit if user might > > get confused. I came up with some names like FEEZE_FAST and > > FREEZE_MINIMAL but I'm not sure these are better. > > FREEZE_FAST seems good. VACUUM (CHILL) ?
On Fri, Nov 20, 2020 at 9:08 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > There are two costs associated with this processing. One is dirtying > the page (which means it needs to be written down when evicted), and the > other is to write WAL records for each change. The cost for the latter > is going to be the same in both cases (with this change and without) > because the same WAL will have to be written -- the only difference is > *when* do you pay it. The cost of the former is quite different; with > Simon's patch we dirty the page once, and without the patch we may dirty > it several times before it becomes "stable" and no more writes are done > to it. > > (If you have tables whose pages change all the time, there would be no > difference with or without the patch.) > > Dirtying the page less times means less full-page images to WAL, too, > which can be significant. Yeah, I think dirtying the page fewer times is a big win. However, a page may have tuples that are not yet all-visible, and we can't freeze those just because we are freezing others. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2020-Nov-20, Robert Haas wrote: > Yeah, I think dirtying the page fewer times is a big win. However, a > page may have tuples that are not yet all-visible, and we can't freeze > those just because we are freezing others. Of course! We should only freeze tuples that are freezable. I thought that was obvious :-)
On Fri, Nov 20, 2020 at 11:02 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2020-Nov-20, Robert Haas wrote: > > Yeah, I think dirtying the page fewer times is a big win. However, a > > page may have tuples that are not yet all-visible, and we can't freeze > > those just because we are freezing others. > > Of course! We should only freeze tuples that are freezable. I thought > that was obvious :-) I didn't mean to imply that anyone in particular thought the contrary. It's just an easy mistake to make when thinking about these kinds of topics. Ask me how I know. It's also easy to forget that both xmin and xmax can require freezing, and that the time at which you can do one can be different than the time at which you can do the other. Or at least, I have found it so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, 20 Nov 2020 at 11:47, Simon Riggs <simon@2ndquadrant.com> wrote: > > On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote: > > > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > > > Patches attached. > > > > > > 1. vacuum_anti_wraparound.v2.patch > > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1) > > > > > > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option. > > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something > > > > > else, but I dislike anti-wraparound. > > > > > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people. > > > > I'm sure a more descriptive term exists. > > > > > > Since we use the term aggressive scan in the docs, I personally don't > > > feel unnatural about that. But since this option also disables index > > > cleanup when not enabled explicitly, I’m concerned a bit if user might > > > get confused. I came up with some names like FEEZE_FAST and > > > FREEZE_MINIMAL but I'm not sure these are better. > > > > FREEZE_FAST seems good. > > > > > BTW if this option also disables index cleanup for faster freezing, > > > why don't we disable heap truncation as well? > > > > Good idea > > Patch attached, using the name "FAST_FREEZE" instead. And the additional patch also. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Fri, 20 Nov 2020 at 15:29, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Note on heap_prepare_freeze_tuple()'s fifth parameter, it's not valid to > pass OldestXmin; you need a multixact limit there, not an Xid limit. I > think the return value of GetOldestMultiXactId is a good choice. AFAICS > this means that you'll need to add a new output argument to > vacuum_set_xid_limits (You *could* call GetOldestMultiXactId again, but > it seems a waste). > > Maybe it's time for vacuum_set_xid_limits to have a caller's-stack- > allocated struct for input and output values, rather than so many > arguments and output pointers to fill. The idea was to maximise freezing because we are already dirtying this data block, so the reasoning doesn't extend directly to multixacts. Being more active with multixacts could easily cause more churn there. So I'm changing the patch to only work with Xids not both xids and multixacts. If this gets committed we can think more about multixacts and whether to optimize freezing them in the same situation or not. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote:
> >
> > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > >
> > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:
> > > > >
> > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > > > Patches attached.
> > > > > > 1. vacuum_anti_wraparound.v2.patch
> > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1)
> > > > >
> > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option.
> > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something
> > > > > else, but I dislike anti-wraparound.
> > > >
> > > > -1 for using the term AGGRESSIVE, which seems likely to offend people.
> > > > I'm sure a more descriptive term exists.
> > >
> > > Since we use the term aggressive scan in the docs, I personally don't
> > > feel unnatural about that. But since this option also disables index
> > > cleanup when not enabled explicitly, I’m concerned a bit if user might
> > > get confused. I came up with some names like FEEZE_FAST and
> > > FREEZE_MINIMAL but I'm not sure these are better.
> >
> > FREEZE_FAST seems good.
> >
> > > BTW if this option also disables index cleanup for faster freezing,
> > > why don't we disable heap truncation as well?
> >
> > Good idea
>
> Patch attached, using the name "FAST_FREEZE" instead.
>
Thank you for updating the patch.
Here are some comments on the patch.
----
- if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
+ if (params->options & VACOPT_DISABLE_PAGE_SKIPPING ||
+ params->options & VACOPT_FAST_FREEZE)
I think we need to update the following comment that is above this
change as well:
/*
* We request an aggressive scan if the table's frozen Xid is now older
* than or equal to the requested Xid full-table scan limit; or if the
* table's minimum MultiXactId is older than or equal to the requested
* mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
*/
This mentions only DISABLE_PAGE_SKIPPING now. Or the second idea is to
set both params.freeze_table_age and params.multixact_freeze_table_age
to 0 at ExecVacuum() instead of getting aggressive turned on here.
Considering the consistency between FREEZE and FREEZE_FAST, we might
want to take the second option.
---
+ if (fast_freeze &&
+ params.index_cleanup == VACOPT_TERNARY_DEFAULT)
+ params.index_cleanup = VACOPT_TERNARY_DISABLED;
+
+ if (fast_freeze &&
+ params.truncate == VACOPT_TERNARY_DEFAULT)
+ params.truncate = VACOPT_TERNARY_DISABLED;
+
+ if (fast_freeze && freeze)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot specify both FREEZE and FAST_FREEZE
options on VACUUM")));
+
I guess that you disallow enabling both FREEZE and FAST_FREEZE because
it's contradictory, which makes sense to me. But it seems to me that
enabling FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE is also
contradictory because it will no longer be “fast”. The purpose of this
option to advance relfrozenxid as fast as possible by disabling index
cleanup, heap truncation etc. Is there any use case where a user wants
to enable these options (FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE) at
the same time? If not, probably it’s better to either disallow it or
have FAST_FREEZE overwrites these two settings even if the user
specifies them explicitly.
Regards,
--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/
Hi Simon, On Mon, Nov 30, 2020 at 1:53 AM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Fri, 20 Nov 2020 at 15:29, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Note on heap_prepare_freeze_tuple()'s fifth parameter, it's not valid to > > pass OldestXmin; you need a multixact limit there, not an Xid limit. I > > think the return value of GetOldestMultiXactId is a good choice. AFAICS > > this means that you'll need to add a new output argument to > > vacuum_set_xid_limits (You *could* call GetOldestMultiXactId again, but > > it seems a waste). > > > > Maybe it's time for vacuum_set_xid_limits to have a caller's-stack- > > allocated struct for input and output values, rather than so many > > arguments and output pointers to fill. > > The idea was to maximise freezing because we are already dirtying this > data block, so the reasoning doesn't extend directly to multixacts. > Being more active with multixacts could easily cause more churn there. > > So I'm changing the patch to only work with Xids not both xids and > multixacts. If this gets committed we can think more about multixacts > and whether to optimize freezing them in the same situation or not. > You sent in your patch, one_freeze_then_max_freeze.v7.patch to pgsql-hackers on Nov 30, but you did not post it to the next CommitFest[1]. If this was intentional, then you need to take no action. However, if you want your patch to be reviewed as part of the upcoming CommitFest, then you need to add it yourself before 2021-01-01 AoE[2]. Thanks for your contributions. Regards, [1] https://commitfest.postgresql.org/31/ [2] https://en.wikipedia.org/wiki/Anywhere_on_Earth -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
On Tue, Dec 1, 2020 at 10:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> >
> > On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote:
> > >
> > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > >
> > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > > > > Patches attached.
> > > > > > > 1. vacuum_anti_wraparound.v2.patch
> > > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1)
> > > > > >
> > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option.
> > > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something
> > > > > > else, but I dislike anti-wraparound.
> > > > >
> > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people.
> > > > > I'm sure a more descriptive term exists.
> > > >
> > > > Since we use the term aggressive scan in the docs, I personally don't
> > > > feel unnatural about that. But since this option also disables index
> > > > cleanup when not enabled explicitly, I’m concerned a bit if user might
> > > > get confused. I came up with some names like FEEZE_FAST and
> > > > FREEZE_MINIMAL but I'm not sure these are better.
> > >
> > > FREEZE_FAST seems good.
> > >
> > > > BTW if this option also disables index cleanup for faster freezing,
> > > > why don't we disable heap truncation as well?
> > >
> > > Good idea
> >
> > Patch attached, using the name "FAST_FREEZE" instead.
> >
>
> Thank you for updating the patch.
>
> Here are some comments on the patch.
>
> ----
> - if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
> + if (params->options & VACOPT_DISABLE_PAGE_SKIPPING ||
> + params->options & VACOPT_FAST_FREEZE)
>
> I think we need to update the following comment that is above this
> change as well:
>
> /*
> * We request an aggressive scan if the table's frozen Xid is now older
> * than or equal to the requested Xid full-table scan limit; or if the
> * table's minimum MultiXactId is older than or equal to the requested
> * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
> */
>
> This mentions only DISABLE_PAGE_SKIPPING now. Or the second idea is to
> set both params.freeze_table_age and params.multixact_freeze_table_age
> to 0 at ExecVacuum() instead of getting aggressive turned on here.
> Considering the consistency between FREEZE and FREEZE_FAST, we might
> want to take the second option.
>
> ---
> + if (fast_freeze &&
> + params.index_cleanup == VACOPT_TERNARY_DEFAULT)
> + params.index_cleanup = VACOPT_TERNARY_DISABLED;
> +
> + if (fast_freeze &&
> + params.truncate == VACOPT_TERNARY_DEFAULT)
> + params.truncate = VACOPT_TERNARY_DISABLED;
> +
> + if (fast_freeze && freeze)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot specify both FREEZE and FAST_FREEZE
> options on VACUUM")));
> +
>
> I guess that you disallow enabling both FREEZE and FAST_FREEZE because
> it's contradictory, which makes sense to me. But it seems to me that
> enabling FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE is also
> contradictory because it will no longer be “fast”. The purpose of this
> option to advance relfrozenxid as fast as possible by disabling index
> cleanup, heap truncation etc. Is there any use case where a user wants
> to enable these options (FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE) at
> the same time? If not, probably it’s better to either disallow it or
> have FAST_FREEZE overwrites these two settings even if the user
> specifies them explicitly.
>
I sent some review comments a month ago but the patch was marked as
"needs review”, which was incorrect So I think "waiting on author" is
a more appropriate state for this patch. I'm switching the patch as
"waiting on author".
Regards,
--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/
Hi Simon,
On Mon, Jan 4, 2021 at 11:45 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Dec 1, 2020 at 10:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > >
> > > On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote:
> > > >
> > > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > > >
> > > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > > > > > Patches attached.
> > > > > > > > 1. vacuum_anti_wraparound.v2.patch
> > > > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1)
> > > > > > >
> > > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option.
> > > > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something
> > > > > > > else, but I dislike anti-wraparound.
> > > > > >
> > > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people.
> > > > > > I'm sure a more descriptive term exists.
> > > > >
> > > > > Since we use the term aggressive scan in the docs, I personally don't
> > > > > feel unnatural about that. But since this option also disables index
> > > > > cleanup when not enabled explicitly, I’m concerned a bit if user might
> > > > > get confused. I came up with some names like FEEZE_FAST and
> > > > > FREEZE_MINIMAL but I'm not sure these are better.
> > > >
> > > > FREEZE_FAST seems good.
> > > >
> > > > > BTW if this option also disables index cleanup for faster freezing,
> > > > > why don't we disable heap truncation as well?
> > > >
> > > > Good idea
> > >
> > > Patch attached, using the name "FAST_FREEZE" instead.
> > >
> >
> > Thank you for updating the patch.
> >
> > Here are some comments on the patch.
> >
> > ----
> > - if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
> > + if (params->options & VACOPT_DISABLE_PAGE_SKIPPING ||
> > + params->options & VACOPT_FAST_FREEZE)
> >
> > I think we need to update the following comment that is above this
> > change as well:
> >
> > /*
> > * We request an aggressive scan if the table's frozen Xid is now older
> > * than or equal to the requested Xid full-table scan limit; or if the
> > * table's minimum MultiXactId is older than or equal to the requested
> > * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
> > */
> >
> > This mentions only DISABLE_PAGE_SKIPPING now. Or the second idea is to
> > set both params.freeze_table_age and params.multixact_freeze_table_age
> > to 0 at ExecVacuum() instead of getting aggressive turned on here.
> > Considering the consistency between FREEZE and FREEZE_FAST, we might
> > want to take the second option.
> >
> > ---
> > + if (fast_freeze &&
> > + params.index_cleanup == VACOPT_TERNARY_DEFAULT)
> > + params.index_cleanup = VACOPT_TERNARY_DISABLED;
> > +
> > + if (fast_freeze &&
> > + params.truncate == VACOPT_TERNARY_DEFAULT)
> > + params.truncate = VACOPT_TERNARY_DISABLED;
> > +
> > + if (fast_freeze && freeze)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > + errmsg("cannot specify both FREEZE and FAST_FREEZE
> > options on VACUUM")));
> > +
> >
> > I guess that you disallow enabling both FREEZE and FAST_FREEZE because
> > it's contradictory, which makes sense to me. But it seems to me that
> > enabling FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE is also
> > contradictory because it will no longer be “fast”. The purpose of this
> > option to advance relfrozenxid as fast as possible by disabling index
> > cleanup, heap truncation etc. Is there any use case where a user wants
> > to enable these options (FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE) at
> > the same time? If not, probably it’s better to either disallow it or
> > have FAST_FREEZE overwrites these two settings even if the user
> > specifies them explicitly.
> >
>
> I sent some review comments a month ago but the patch was marked as
> "needs review”, which was incorrect So I think "waiting on author" is
> a more appropriate state for this patch. I'm switching the patch as
> "waiting on author".
>
Status update for a commitfest entry.
This entry has been "Waiting on Author" status and the patch has not
been updated since Nov 30. Are you still planning to work on this?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > This entry has been "Waiting on Author" status and the patch has not > been updated since Nov 30. Are you still planning to work on this? Yes, new patch version tomorrow. Thanks for the nudge and the review. -- Simon Riggs http://www.EnterpriseDB.com/
On Fri, 12 Mar 2021 at 22:16, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: > > On 1/28/21 2:33 PM, Simon Riggs wrote: > > On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > >> This entry has been "Waiting on Author" status and the patch has not > >> been updated since Nov 30. Are you still planning to work on this? > > > > Yes, new patch version tomorrow. Thanks for the nudge and the review. > > > > So, is it tomorrow already? ;-) Been a long day. ;-) -- Simon Riggs http://www.EnterpriseDB.com/
On 3/17/21 4:50 PM, Simon Riggs wrote: > On Fri, 12 Mar 2021 at 22:16, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: >> >> On 1/28/21 2:33 PM, Simon Riggs wrote: >>> On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >>> >>>> This entry has been "Waiting on Author" status and the patch has not >>>> been updated since Nov 30. Are you still planning to work on this? >>> >>> Yes, new patch version tomorrow. Thanks for the nudge and the review. >>> >> >> So, is it tomorrow already? ;-) > > Been a long day. ;-) It has been five months since this patch was updated, so marking Returned with Feedback. Please resubmit to the next CF when you have a new patch. Regards, -- -David david@pgmasters.net
On Thu, 8 Apr 2021 at 16:23, David Steele <david@pgmasters.net> wrote: > > On 3/17/21 4:50 PM, Simon Riggs wrote: > > On Fri, 12 Mar 2021 at 22:16, Tomas Vondra > > <tomas.vondra@enterprisedb.com> wrote: > >> > >> On 1/28/21 2:33 PM, Simon Riggs wrote: > >>> On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > >>> > >>>> This entry has been "Waiting on Author" status and the patch has not > >>>> been updated since Nov 30. Are you still planning to work on this? > >>> > >>> Yes, new patch version tomorrow. Thanks for the nudge and the review. > >>> > >> > >> So, is it tomorrow already? ;-) > > > > Been a long day. ;-) > > It has been five months since this patch was updated, so marking > Returned with Feedback. > > Please resubmit to the next CF when you have a new patch. There are 2 separate patch-sets on this thread, with separate CF entries. One has requested changes that have not been made. I presume this one has been RwF'd. What is happening about the other patch? -- Simon Riggs http://www.EnterpriseDB.com/
On 4/8/21 11:41 AM, Simon Riggs wrote: > On Thu, 8 Apr 2021 at 16:23, David Steele <david@pgmasters.net> wrote: >> >> On 3/17/21 4:50 PM, Simon Riggs wrote: >>> On Fri, 12 Mar 2021 at 22:16, Tomas Vondra >>> <tomas.vondra@enterprisedb.com> wrote: >>>> >>>> On 1/28/21 2:33 PM, Simon Riggs wrote: >>>>> On Thu, 28 Jan 2021 at 12:53, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >>>>> >>>>>> This entry has been "Waiting on Author" status and the patch has not >>>>>> been updated since Nov 30. Are you still planning to work on this? >>>>> >>>>> Yes, new patch version tomorrow. Thanks for the nudge and the review. >>>>> >>>> >>>> So, is it tomorrow already? ;-) >>> >>> Been a long day. ;-) >> >> It has been five months since this patch was updated, so marking >> Returned with Feedback. >> >> Please resubmit to the next CF when you have a new patch. > > There are 2 separate patch-sets on this thread, with separate CF entries. > > One has requested changes that have not been made. I presume this one > has been RwF'd. > > What is happening about the other patch? Hmmm, well https://commitfest.postgresql.org/32/2908 and https://commitfest.postgresql.org/32/2909 are both linked to the same thread with the same patch, so I thought it was a duplicate. It's not clear to me which patch is which, so perhaps move one CF entry to next CF and clarify which patch is current? Regards, -- -David david@pgmasters.net
On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > >> It has been five months since this patch was updated, so marking > >> Returned with Feedback. > >> > >> Please resubmit to the next CF when you have a new patch. > > > > There are 2 separate patch-sets on this thread, with separate CF entries. > > > > One has requested changes that have not been made. I presume this one > > has been RwF'd. > > > > What is happening about the other patch? > > Hmmm, well https://commitfest.postgresql.org/32/2908 and > https://commitfest.postgresql.org/32/2909 are both linked to the same > thread with the same patch, so I thought it was a duplicate. Nobody had mentioned any confusion. Multiple patches on one thread is common. > It's not clear to me which patch is which, so perhaps move one CF entry > to next CF and clarify which patch is current? Entry: Maximize page freezing has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. one_freeze_then_max_freeze.v7.patch Other patch is awaiting changes. -- Simon Riggs http://www.EnterpriseDB.com/
On 4/8/21 12:29 PM, Simon Riggs wrote: > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > >>>> It has been five months since this patch was updated, so marking >>>> Returned with Feedback. >>>> >>>> Please resubmit to the next CF when you have a new patch. >>> >>> There are 2 separate patch-sets on this thread, with separate CF entries. >>> >>> One has requested changes that have not been made. I presume this one >>> has been RwF'd. >>> >>> What is happening about the other patch? >> >> Hmmm, well https://commitfest.postgresql.org/32/2908 and >> https://commitfest.postgresql.org/32/2909 are both linked to the same >> thread with the same patch, so I thought it was a duplicate. > > Nobody had mentioned any confusion. Multiple patches on one thread is common. Yes, but these days they generally get updated in the same reply so the cfbot can test them all. In your case only the latest patch was being tested and it was not applying cleanly. >> It's not clear to me which patch is which, so perhaps move one CF entry >> to next CF and clarify which patch is current? > > Entry: Maximize page freezing > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > one_freeze_then_max_freeze.v7.patch That CF entry was marked Waiting for Author on 3/11, so I thought it was for this patch. In fact, both CF entries were Waiting for Author for some time. Moved to the next CF in Waiting for Author state. Regards, -- -David david@pgmasters.net
On Thu, 8 Apr 2021 at 17:44, David Steele <david@pgmasters.net> wrote: > > On 4/8/21 12:29 PM, Simon Riggs wrote: > > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > > > >>>> It has been five months since this patch was updated, so marking > >>>> Returned with Feedback. > >>>> > >>>> Please resubmit to the next CF when you have a new patch. > >>> > >>> There are 2 separate patch-sets on this thread, with separate CF entries. > >>> > >>> One has requested changes that have not been made. I presume this one > >>> has been RwF'd. > >>> > >>> What is happening about the other patch? > >> > >> Hmmm, well https://commitfest.postgresql.org/32/2908 and > >> https://commitfest.postgresql.org/32/2909 are both linked to the same > >> thread with the same patch, so I thought it was a duplicate. > > > > Nobody had mentioned any confusion. Multiple patches on one thread is common. > > Yes, but these days they generally get updated in the same reply so the > cfbot can test them all. In your case only the latest patch was being > tested and it was not applying cleanly. > > >> It's not clear to me which patch is which, so perhaps move one CF entry > >> to next CF and clarify which patch is current? > > > > Entry: Maximize page freezing > > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > > one_freeze_then_max_freeze.v7.patch > > That CF entry was marked Waiting for Author on 3/11, so I thought it was > for this patch. In fact, both CF entries were Waiting for Author for > some time. That was done by someone that hadn't mentioned it to me, or the list. It is not Waiting on Author. > Moved to the next CF in Waiting for Author state. That is clearly an incorrect state. -- Simon Riggs http://www.EnterpriseDB.com/
On 2021-Apr-08, Simon Riggs wrote: > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > > It's not clear to me which patch is which, so perhaps move one CF entry > > to next CF and clarify which patch is current? > > Entry: Maximize page freezing > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > one_freeze_then_max_freeze.v7.patch Oh dear, was this waiting on me? It was not in my to-do, so I didn't pay close attention. -- Álvaro Herrera 39°49'30"S 73°17'W
On Thu, 8 Apr 2021 at 18:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Apr-08, Simon Riggs wrote: > > > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > > > > It's not clear to me which patch is which, so perhaps move one CF entry > > > to next CF and clarify which patch is current? > > > > Entry: Maximize page freezing > > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > > one_freeze_then_max_freeze.v7.patch > > Oh dear, was this waiting on me? It was not in my to-do, so I didn't > pay close attention. No problem, if I felt the idea deserved priority attention I would have pinged you. I think I'll open a new thread later to allow it to be discussed without confusion. -- Simon Riggs http://www.EnterpriseDB.com/
On Thu, Apr 8, 2021 at 11:40 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Thu, 8 Apr 2021 at 18:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > On 2021-Apr-08, Simon Riggs wrote: > > > > > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > > > > > > It's not clear to me which patch is which, so perhaps move one CF entry > > > > to next CF and clarify which patch is current? > > > > > > Entry: Maximize page freezing > > > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > > > one_freeze_then_max_freeze.v7.patch > > > > Oh dear, was this waiting on me? It was not in my to-do, so I didn't > > pay close attention. > > No problem, if I felt the idea deserved priority attention I would > have pinged you. > > I think I'll open a new thread later to allow it to be discussed > without confusion. The patch no longer applies on the head, please post a rebased patch. Regards, Vignesh
On Wed, 14 Jul 2021 at 17:22, vignesh C <vignesh21@gmail.com> wrote: > > On Thu, Apr 8, 2021 at 11:40 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > > > On Thu, 8 Apr 2021 at 18:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > > > On 2021-Apr-08, Simon Riggs wrote: > > > > > > > On Thu, 8 Apr 2021 at 16:58, David Steele <david@pgmasters.net> wrote: > > > > > > > > It's not clear to me which patch is which, so perhaps move one CF entry > > > > > to next CF and clarify which patch is current? > > > > > > > > Entry: Maximize page freezing > > > > has this patch, perfectly fine, awaiting review from Alvaro since 29 Nov. > > > > one_freeze_then_max_freeze.v7.patch > > > > > > Oh dear, was this waiting on me? It was not in my to-do, so I didn't > > > pay close attention. > > > > No problem, if I felt the idea deserved priority attention I would > > have pinged you. > > > > I think I'll open a new thread later to allow it to be discussed > > without confusion. > > The patch no longer applies on the head, please post a rebased patch. I've withdrawn this patch/thread to avoid further confusion. -- Simon Riggs http://www.2ndQuadrant.com/