On Thu, Aug 13, 2026 at 11:41:59AM -0500, Nathan Bossart wrote: > On Thu, Aug 13, 2026 at 03:28:23PM +0900, Michael Paquier wrote: >> In 0004, I was wondering if this makes the code weaker on some >> aspects, because we are switching from a logic where we always had >> an entry in the mapping hashtable for a main relation with a TOAST >> table to a logic where a NULL entry could mean either:
Looking at the five remaining patches in v13, replying to the message where v12 was posted. > I personally don't see much point in tracking additional information we > don't need. We can already tell if the table in question is a TOAST table, > so a missing entry in the hash table means that we didn't find any main > table relopts for it. *shrug* Hmm. Okay. Fine by me at the end. >> + if (rel->rd_options) >> + memcpy(&relopts_copy, rel->rd_options, sizeof(StdRdOptions)); >> [...] >> + * NB: This destructively modifies toast_opts, and what it returns may be >> + * either argument, so the caller must know which of the two it owns. >> >> Hmm. I am not really cool with this as an API contract. That can >> bite. That's not re-entrant, to begin with, and on top of that this >> function returns the merged result. It would be saner to create a >> copy, and return the copy as a result, copy that we do anyway before >> the sole caller of the function with a memcpy(). :) > > Done in v12. The API contract in v13-0004 looks much better to me now. No more overwrites of the inputs. It's almost like you could add some const markers. >> Hmm. We have three callers of get_effective_relopts(), and some paths >> can call it for a main relation, meaning that the >> merge_toast_reloptions() makes little sense because there is nothing >> to merge. Should this enforce a check so as we try to merge >> reloptions only when dealing with a toast relation, or should the >> callers for that by themselves based on the classForm->relkind? > > It enforces that already. The relkind check in the function ensures that > main_opts is always NULL for non-TOAST relations, and > merge_toast_reloptions() always returns the first argument when the second > is NULL. I do think this could be called out a bit better, which I've > tried to do in v12. At the end of the day, get_effective_relopts() acts as a thin wrapper of extractRelOptions(), merging two existing code patterns and re-using the same pattern for the scoring. Perhaps "effective" is the term that troubles me here, while having merge_toast_reloptions(). You need the merge_*() for the vacuum part, but I'm also wondering if this could not be reworked with less routines overall. I don't have a clean idea on top of my mind now, and that does not count as an objection. This gives an impression of being slightly overcomplicated. >> In 0008, some tests would be nice for the autovacuum case, at least. >> That would mean a TAP test to check a bit what do_autovacuum() does, >> and now the SQL test in injection_points only looks after >> pg_stat_get_autovacuum_scores(). I am honestly puzzled by the reason >> why this is added inside injection_points at all. There is no >> dependency to a point, and no new information with the NOTICE >> messages. A better location would fit better the purpose of the score >> test. > > I only put it there because 0007 added a similar test, and 0007 and 0008 > used to be one patch. In v12, I've tried my hand at a TAP test. The test looks pretty nice here. -- Michael
signature.asc
Description: PGP signature
