public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/5] Allow composite types in bootstrap 4+ messages / 4 participants [nested] [flat]
* [PATCH 2/5] Allow composite types in bootstrap @ 2020-11-17 15:28 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-11-17 15:28 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 18eb62ca47..e4fc75ab84 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -916,6 +916,7 @@ gettype(char *type) { if (Typ != NIL) { + static bool did_reread PG_USED_FOR_ASSERTS_ONLY = false; /* Already reread pg_types */ ListCell *lc; foreach (lc, Typ) @@ -927,6 +928,33 @@ gettype(char *type) return app->am_oid; } } + + /* + * The type wasn't known; check again to handle composite + * types, added since first populating the array. + */ + + /* + * Once all the types are populated and we handled composite + * types, shouldn't need to do that again. + */ + Assert(!did_reread); + did_reread = true; + + list_free_deep(Typ); + Typ = NULL; + populate_typ_array(); + + /* Need to avoid infinite recursion... */ + foreach (lc, Typ) + { + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) + { + Ap = app; + return app->am_oid; + } + } } else { -- 2.26.2 --------------614DDB87AFFED893713AC0E9 Content-Type: text/x-patch; charset=UTF-8; name="0003-Extended-statistics-on-expressions-20210304.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0003-Extended-statistics-on-expressions-20210304.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Confine vacuum skip logic to lazy_scan_skip @ 2024-03-08 13:49 Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Heikki Linnakangas @ 2024-03-08 13:49 UTC (permalink / raw) To: vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Nazir Bilal Yavuz <[email protected]>; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]> On 08/03/2024 02:46, Melanie Plageman wrote: > On Wed, Mar 06, 2024 at 10:00:23PM -0500, Melanie Plageman wrote: >>> I feel heap_vac_scan_get_next_block() function could use some love. Maybe >>> just some rewording of the comments, or maybe some other refactoring; not >>> sure. But I'm pretty happy with the function signature and how it's called. > > I've cleaned up the comments on heap_vac_scan_next_block() in the first > couple patches (not so much in the streaming read user). Let me know if > it addresses your feelings or if I should look for other things I could > change. Thanks, that is better. I think I now finally understand how the function works, and now I can see some more issues and refactoring opportunities :-). Looking at current lazy_scan_skip() code in 'master', one thing now caught my eye (and it's the same with your patches): > *next_unskippable_allvis = true; > while (next_unskippable_block < rel_pages) > { > uint8 mapbits = visibilitymap_get_status(vacrel->rel, > next_unskippable_block, > vmbuffer); > > if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) == 0) > { > Assert((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0); > *next_unskippable_allvis = false; > break; > } > > /* > * Caller must scan the last page to determine whether it has tuples > * (caller must have the opportunity to set vacrel->nonempty_pages). > * This rule avoids having lazy_truncate_heap() take access-exclusive > * lock on rel to attempt a truncation that fails anyway, just because > * there are tuples on the last page (it is likely that there will be > * tuples on other nearby pages as well, but those can be skipped). > * > * Implement this by always treating the last block as unsafe to skip. > */ > if (next_unskippable_block == rel_pages - 1) > break; > > /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */ > if (!vacrel->skipwithvm) > { > /* Caller shouldn't rely on all_visible_according_to_vm */ > *next_unskippable_allvis = false; > break; > } > > /* > * Aggressive VACUUM caller can't skip pages just because they are > * all-visible. They may still skip all-frozen pages, which can't > * contain XIDs < OldestXmin (XIDs that aren't already frozen by now). > */ > if ((mapbits & VISIBILITYMAP_ALL_FROZEN) == 0) > { > if (vacrel->aggressive) > break; > > /* > * All-visible block is safe to skip in non-aggressive case. But > * remember that the final range contains such a block for later. > */ > skipsallvis = true; > } > > /* XXX: is it OK to remove this? */ > vacuum_delay_point(); > next_unskippable_block++; > nskippable_blocks++; > } Firstly, it seems silly to check DISABLE_PAGE_SKIPPING within the loop. When DISABLE_PAGE_SKIPPING is set, we always return the next block and set *next_unskippable_allvis = false regardless of the visibility map, so why bother checking the visibility map at all? Except at the very last block of the relation! If you look carefully, at the last block we do return *next_unskippable_allvis = true, if the VM says so, even if DISABLE_PAGE_SKIPPING is set. I think that's wrong. Surely the intention was to pretend that none of the VM bits were set if DISABLE_PAGE_SKIPPING is used, also for the last block. This was changed in commit 980ae17310: > @@ -1311,7 +1327,11 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block, > > /* DISABLE_PAGE_SKIPPING makes all skipping unsafe */ > if (!vacrel->skipwithvm) > + { > + /* Caller shouldn't rely on all_visible_according_to_vm */ > + *next_unskippable_allvis = false; > break; > + } Before that, *next_unskippable_allvis was set correctly according to the VM, even when DISABLE_PAGE_SKIPPING was used. It's not clear to me why that was changed. And I think setting it to 'true' would be a more failsafe value than 'false'. When *next_unskippable_allvis is set to true, the caller cannot rely on it because a concurrent modification could immediately clear the VM bit. But because VACUUM is the only process that sets VM bits, if it's set to false, the caller can assume that it's still not set later on. One consequence of that is that with DISABLE_PAGE_SKIPPING, lazy_scan_heap() dirties all pages, even if there are no changes. The attached test script demonstrates that. ISTM we should revert the above hunk, and backpatch it to v16. I'm a little wary because I don't understand why that change was made in the first place, though. I think it was just an ill-advised attempt at tidying up the code as part of the larger commit, but I'm not sure. Peter, do you remember? I wonder if we should give up trying to set all_visible_according_to_vm correctly when we decide what to skip, and always do "all_visible_according_to_vm = visibilitymap_get_status(...)" in lazy_scan_prune(). It would be more expensive, but maybe it doesn't matter in practice. It would get rid of this tricky bookkeeping in heap_vac_scan_next_block(). -- Heikki Linnakangas Neon (https://neon.tech) Attachments: [application/sql] vactest.sql (844B, ../../[email protected]/2-vactest.sql) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Confine vacuum skip logic to lazy_scan_skip @ 2024-03-08 15:40 Peter Geoghegan <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Peter Geoghegan @ 2024-03-08 15:40 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Nazir Bilal Yavuz <[email protected]>; Robert Haas <[email protected]> On Fri, Mar 8, 2024 at 8:49 AM Heikki Linnakangas <[email protected]> wrote: > ISTM we should revert the above hunk, and backpatch it to v16. I'm a > little wary because I don't understand why that change was made in the > first place, though. I think it was just an ill-advised attempt at > tidying up the code as part of the larger commit, but I'm not sure. > Peter, do you remember? I think that it makes sense to set the VM when indicated by lazy_scan_prune, independent of what either the visibility map or the page's PD_ALL_VISIBLE marking say. The whole point of DISABLE_PAGE_SKIPPING is to deal with VM corruption, after all. In retrospect I didn't handle this particular aspect very well in commit 980ae17310. The approach I took is a bit crude (and in any case slightly wrong in that it is inconsistent in how it handles the last page). But it has the merit of fixing the case where we just have the VM's all-frozen bit set for a given block (not the all-visible bit set) -- which is always wrong. There was good reason to be concerned about that possibility when 980ae17310 went in. -- Peter Geoghegan ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Confine vacuum skip logic to lazy_scan_skip @ 2024-03-08 15:48 Melanie Plageman <[email protected]> parent: Peter Geoghegan <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Melanie Plageman @ 2024-03-08 15:48 UTC (permalink / raw) To: Peter Geoghegan <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Nazir Bilal Yavuz <[email protected]>; Robert Haas <[email protected]> On Fri, Mar 8, 2024 at 10:41 AM Peter Geoghegan <[email protected]> wrote: > > On Fri, Mar 8, 2024 at 8:49 AM Heikki Linnakangas <[email protected]> wrote: > > ISTM we should revert the above hunk, and backpatch it to v16. I'm a > > little wary because I don't understand why that change was made in the > > first place, though. I think it was just an ill-advised attempt at > > tidying up the code as part of the larger commit, but I'm not sure. > > Peter, do you remember? > > I think that it makes sense to set the VM when indicated by > lazy_scan_prune, independent of what either the visibility map or the > page's PD_ALL_VISIBLE marking say. The whole point of > DISABLE_PAGE_SKIPPING is to deal with VM corruption, after all. Not that it will be fun to maintain another special case in the VM update code in lazy_scan_prune(), but we could have a special case that checks if DISABLE_PAGE_SKIPPING was passed to vacuum and if all_visible_according_to_vm is true and all_visible is true, we update the VM but don't dirty the page. The docs on DISABLE_PAGE_SKIPPING say it is meant to deal with VM corruption -- it doesn't say anything about dealing with incorrectly set PD_ALL_VISIBLE markings. - Melanie ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-03-08 15:48 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 15:28 [PATCH 2/5] Allow composite types in bootstrap Justin Pryzby <[email protected]> 2024-03-08 13:49 Re: Confine vacuum skip logic to lazy_scan_skip Heikki Linnakangas <[email protected]> 2024-03-08 15:40 ` Re: Confine vacuum skip logic to lazy_scan_skip Peter Geoghegan <[email protected]> 2024-03-08 15:48 ` Re: Confine vacuum skip logic to lazy_scan_skip Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox