Re: Refactor how we form HeapTuples for CatalogTuple(Insert|Update)

On Fri, Jan 30, 2026, at 1:54 PM, Tom Lane wrote:
> "Greg Burd" <greg(at)burd(dot)me> writes:
>> On Fri, Jan 30, 2026, at 12:20 PM, Andres Freund wrote:
>>> Sorry for being a downer - but: Is the gain here really worth the squeeze?
>
>> You are certainly not wrong, there are a lot of changes in this patch set. To be honest, I'm not sure if it is "worth the squeeze" either.
>
>> What this patch does: it removes the need to rediscover the set of indexed attributes that changed during catalog tuple updates.
>
>> Impact of that change: as-yet-unmeasured performance gains due to not having to redo work while holding a lock on the heap page.

Hey Tom, thanks for chiming in on this one!

> Keep in mind that a patch like this
>
> * requires a ton of work to review
> * breaks a lot of pending patches
> * breaks a lot of extensions
> * causes significant back-patching pain for the next five years

Yes, I figured as much. And that's a big ask, I get that and I think I acknowledged that in my opening email on this thread. :)

> With that in mind, you'd have to show *massive* performance
> improvements for it to have any chance of getting accepted.
> Frankly I doubt it can possibly clear that bar.

You and me both, there won't be *massive* performance gains from this.

> You could lower the bar considerably if, instead of forcing a
> system-wide API change, you maintained compatibility with the
> existing API and introduced a new one alongside it, using the
> new one only in selected code paths that seem particularly
> performance-critical. That would remove the objection of
> breaking extensions and pending patches, and probably greatly
> reduce the other two costs as well, depending on how selective
> you are about where to introduce the new API.

Certainly, that's one path forward. I don't think I went down this path targeting performance. This was more a way to show that my other patch (CF-5556) didn't overlook the catalog tuple side of things.

I'll review the core of this idea and see if I can boil it down to something we can use on performance-critical paths, maybe updating statistics would benefit.

> With that in mind, I really wonder why it's so critical to change
> from replacement flags represented as array-of-bool to replacement
> flags represented as a Bitmapset. Seems like those are fundamentally
> equivalent. A Bitmapset is more compact, sure, but I fail to see
> why we care about that in this context: the replacement flags are
> typically function-local variables that won't live very long.
> I'm also concerned that the palloc required to create a Bitmapset
> will be a net drag on performance compared to a local array.
>
> regards, tom lane

Tom, I wasn't perfectly upfront with my intentions for this patch. I'll be better at that in the future. The use of a Bitmapset is really a foreshadowing of how I'd like to eventually restructure the HOT updates and at some point introduce a refreshed PHOT (or WARM-like) solution for heap updates. The thought was to use the Bitmapset as a filter in when choosing which indexes required updates.

As for the palloc(), yes that's unfortunate when substituting in a Bitmapset and does add some overhead. I'd toyed with a way to statically allocate a Bitmapset, but that just violated some of the basics of that data structure (NULL for empty being the primary one).

The lesson there is to not get ahead of myself. I'm going to pull the plug on this patch.

thanks all.

-greg