On Fri, Nov 14, 2025 at 10:31:28AM -0500, Greg Burd wrote: > Catalog tuples have knowledge of what attributes are mutated implicitly, > but they don't preserve that information and so > HeapDetermineColumnsInfo() has to re-discover that later on (while the > page is locked). While on the surface this isn't such a big deal, it's > been that way and worked well for years I have other motivations (see > [1] again) for improving this.
Yeah, I've heard that this has been also discussed at the New York
pgconf, where designs were mentioned so as we do not execute
expressions while locking pages. One issue is that this could easily
deadlock if an expression has the idea to re-read the same page we are
locking. So I have a summary of what you have been doing in mind, a
very rough one I suspect.
> That turned out to be a bit of a challenge as we have a lot of places
> where catalog information is updated. We also have two different
> methods for updating said information. And we intermix them. At times
> we hack our way to a solution and ignore convention. I went down the
> rabbit hole on this one, but I'm back up for a cup of tea because what
> I've done seems materially better to me and I've accomplished the goal
> (and can eventually make use of the result in [1]). Is this patch
> useful even without that other work? I think so, just on the basis of
> cleanup. Let me explain.
It's a mix of historical and individual commit style, so having things
slightly different depending on the catalog code path is not a
surprise, at least here.
> [... Two methods for heap tuple updates ... ]
>
> tuple = heap_modify_tuple(tuple, desc, values, nulls, replaces);
> CatalogTupleUpdate(pg_type_desc, &tuple->t_self, tuple);
As far as I can see after applying 0002, 90%-ish of the existing
callers of heap_modify_tuple() are updated to use heap_update_tuple().
That's interesting to see.
> heap_update_tuple() is functionally equivalent to heap_modify_tuple(),
> but takes a Bitmapset called "updated" rather than an array of bool
> (generally) called "replaces" as a method for indicating what was
> modified. Additionally, this new function tries to balance the
> trade-offs of calling heap_getattr() versus heap_deform_tuple() based on
> the ratio of attributes updated and their known runtime complexities.
> Both paths are functionally equivalent.
I don't think that we can completely remove heap_update_tuple() as
code path used for the catalogs updates, but we can get very close to
that. Perhaps we should document at the top of heap_update_tuple()
that developers should not use this API for catalog changes, switching
to the bitmap interface instead in priority?
> * Performance strategy:
> * - If updating many attributes (> 2*natts/3), use heap_getattr() to extract
> * only the few non-updated attributes. This is O(k*n) where k is the number
> * of non-updated attributes, which is small when updating many.
> * - If updating few attributes (<= 2*natts/3), use heap_deform_tuple() to
> * extract all attributes at once (O(n)), then replace the updated ones.
> * This avoids the O(n^2) cost of many heap_getattr() calls.
> *
> * The threshold of 2*natts/3 balances the fixed O(n) cost of
> heap_deform_tuple
> * against the variable O(k*n) cost of heap_getattr, where k = natts -
> num_updated.
This change has been puzzling me quite a bit. Why this choice of
formula with 2/3 of arguments? The maximum number of arguments is
1400 by design, so would one choice matter more than the other in most
cases? Catalogs don't have that many arguments either, so I am not
sure if we need to care with this amount of tuning, TBH, but I am OK
to be wrong.
> indexing.c: has the change to CatalogTupleUpdate/Insert and removal of
> their "WithInfo" companions. If that second part is controversial then
> I don't mind reverting it, but IMO this is cleaner and might either
> inspire more call sites to create and reuse CatalogIndexState or for
> someone (me?) to address the comment that pre-dates my work:
I'm OK with the simplification on this one, but let's make that a
separate patch extracted from 0001. The changes with
CatalogTupleUpdate() and CatalogTupleInsert() don't have to be
included with the changes that introduce a bitmap integration to track
the attributes that are updated. This is pointing to the fact that
there are not that many callers of the WithInfo() flavors in the tree,
compared to the non-info ones (24 AFAIK, vs 160-ish for the Insert
path it seems, for example).
> pg_aggregate.c: in AggregateCreate() we run into a "compromise" pattern
> where the code code either update or insert. In such cases hackers
> should use the macros for update, not insert, everywhere. This records
> the additional information for the update path and doesn't burden the
> insert path with much more than a Bitmapset to free.
Hmm. Fine by me. The bitmap is not used for an INSERT path, so it's
a bit of a waste, but I can see why you have done so to satisfy the
"replace" case, from CREATE OR REPLACE (never liked this kind of
grammar as it can be impredictible implementation-wise with syscache
lookups required, just a personal rant).
Another thing that you could do here is enforce a check in a couple of
places where CatalogTupleInsert() is called for the "updated" bitmap:
all the bits in the map should be set, like in OperatorCreate(). That
may catch bugs at runtime perhaps when adding fields when dealing with
an insert instead of an update?
> pg_constraint.c: brings us to namestrcpy(), take a look at
> RenameConstraintById(). When you run into namestrcpy() you've likely
> just done the mutation to the form directly but you'll want to mark that
> field as updated, as in:
>
> alter.c: take a look at AlterObjectRename_internal() which uses
> get_object_attnum_name() to get the AttrNumber that is to be altered.
> This is fine, but it's not something we can just plug into a macro and
> get the proper name expansion. So, we do it the manual way and add a comment:
Not surprising to have exceptions like that. This is a global path
used for the renames of a bunch of objects. This makes me wonder if
we really have to absolutely change all the code paths that currently
use heap_modify_tuple(). Like this one in alter.c for renames, the
change is not an improvement in readability. Same for
AlterObjectNamespace_internal(). These would live better if still
based on heap_modify_tuple(), because of the fact that they can be fed
several object types.
> analyze.c: uses the HeapTupleSetAllColumnsUpdated() at the start of the
> update_attstats() function to match the replaced code. Later there's
> another interesting deviation from the normal pattern in that function
> where we want to update the nth stakind/opt/coll this was frustrating at
> first until I found that this worked:
>
> HeapTupleSetValue(pg_statistic, stakind1 + k,
> Int16GetDatum(stats->stakind[k]), values);
I'm getting concerned about HeapTupleSetAllColumnsUpdated() while
reaching this part of your message, FWIW.
> So, we're taking the attribute number adding "k" then subtracting 1, and
> voila "Bob's your uncle." While a bit unconventional, I think the
> resulting form is understandable, much more than before which was:
>
> i = Anum_pg_statistic_stakind1 - 1;
> for (k = 0; k < STATISTIC_NUM_SLOTS; k++)
> values[i++] = Int16GetDatum(stats->stakind[k]); /* stakindN */
And more concerns. This change makes me question how wise it is to
have HeapTupleUpdateSetAllColumnsUpdated(), actually. Couldn't this
encourage bugs if somebody forgets to update some of the fields in the
new tuple, still the bitmap has been updated with a range to mark all
the attributes as updated. Having HeapTupleUpdateField() do an update
of the bitmap makes sure that the bitmap and the fields updated are
never out-of-sync. I'm OK with marking the individual attributes as
updated. Having a more aggressive API that marks all of them as
updated sounds dangerous to me.
> 0002 - Update the remainder of catalog updates using the new APIs
>
> This is a lot of applying the pattern over and over until it all works.
> That's it. Nothing much more interesting here.
>
> This is a lot of change, I get that, I think there's value in change
> when the result is cleaner and more maintainable. The one place I'll
> agree up front where this might case problems is back-patching fixes.
> That'll have to have the new approach in some branches and the old
> approach in others. It's a 5-year commitment, I don't take that lightly.
Backpatches imply catalog tuple manipulations from time to time, so
that would create noise, yes.
> If you made it this far, I owe you a beer/coffee/token-of-appreciation.
Sentence noted.
> Without this patch my [1] will have an ugly wart
> (HeapDetermineColumnsInfo()) for the CatalogTupleUpdate path only. I
> can live with that, I'd rather not.
It seems to me that this answer is better answered as "rather not",
especially if this improves the code maintenance in the long-term.
Patch 0001 could be split into more pieces, particularly regarding the
argument it makes about the fact that there are few callers of
CatalogTuplesMultiInsertWithInfo(), CatalogTupleInsertWithInfo() and
CatalogTupleUpdateWithInfo(), where we can just plug NULL for the
index state. I'd suggest to make these routines leaner in a first
patch.
/* No, insert new tuple */
stup = heap_form_tuple(RelationGetDescr(sd), values, nulls);
- CatalogTupleInsertWithInfo(sd, stup, indstate);
+ CatalogTupleInsert(sd, stup, NULL);
}
This one in update_attstats() looks wrong. Why are you passing NULL
instead of an opened index state? We keep track of indstate for all
the pg_statistic tuples updated.
HeapTupleSetField() is used nowhere. Do we really need it?
--
Michael
signature.asc
Description: PGP signature
