Re: ExecRTCheckPerms() and many prunable partitions - Mailing list pgsql-hackers

On Sat, Nov 12, 2022 at 1:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > On 2022-Oct-06, Amit Langote wrote:
> >> Actually, List of Bitmapsets turned out to be something that doesn't
> >> just-work with our Node infrastructure, which I found out thanks to
> >> -DWRITE_READ_PARSE_PLAN_TREES.  So, I had to go ahead and add
> >> first-class support for copy/equal/write/read support for Bitmapsets,
>
> > Hmm, is this related to what Tom posted as part of his 0004 in
> > https://postgr.es/m/2901865.1667685211@sss.pgh.pa.us
>
> It could be.  For some reason I thought that Amit had withdrawn
> his proposal to make Bitmapsets be Nodes.

I think you are referring to [1] that I had forgotten to link to here.
I did reimplement a data structure in my patch on the "Re: generic
plans and initial pruning" thread to stop using a List of Bitmapsets,
so the Bitmapset as Nodes functionality became unnecessary there,
though I still need it for the proposal here to move
extraUpdatedColumns (patch 0004) into ModifyTable node.

> The code I was using that for would rather have fixed-size arrays
> of Bitmapsets than variable-size Lists, mainly because it always
> knows ab initio what the max length of the array will be.  But
> I don't think that the preference is so strong that it justifies
> a private data structure.
>
> The main thing I was wondering about in connection with that
> was whether to assume that there could be other future applications
> of the logic to perform multi-bitmapset union, intersection,
> etc.  If so, then I'd be inclined to choose different naming and
> put those functions in or near to bitmapset.c.  It doesn't look
> like Amit's code needs anything like that, but maybe somebody
> has an idea about other applications?

Yes, simple storage of multiple Bitmapsets in a List somewhere in a
parse/plan tree sounded like that would have wider enough use to add
proper node support for.   Assuming you mean trying to generalize
VarAttnoSet in your patch 0004 posted at [2], I wonder if you want to
somehow make its indexability by varno / RT index a part of the
interface of the generic code you're thinking for it?  For example,
varattnoset_*_members collection of routines in that patch seem to
assume that the Bitmapsets at a given index in the provided pair of
VarAttnoSets are somehow related -- covering to the same base relation
in this case.  That does not sound very generalizable but maybe that
is not what you are thinking at all.

> Anyway, I concur with Peter's upthread comment that making
> Bitmapsets be Nodes is probably justifiable all by itself.
> The lack of a Node tag in them now is just because in a 32-bit
> world it seemed like unnecessary bloat.  But on 64-bit machines
> it's free, and we aren't optimizing for 32-bit any more.
>
> I do not like the details of v24-0003 at all though, because
> it seems to envision that a "node Bitmapset" is a different
> thing from a raw Bitmapset.  That can only lead to bugs ---
> why would we not make it the case that every Bitmapset is
> properly labeled with the node tag?

Yeah, I just didn't think hard enough to realize that having
bitmapset.c itself set the node tag is essentially free, and it looks
like a better design anyway than the design where callers get to
choose to make the bitmapset they are manipulating a Node or not.

> Also, although I'm on board with making Bitmapsets be Nodes,
> I don't think I'm on board with changing their dump format.
> Planner node dumps would get enormously bulkier and less
> readable if we changed things like
>
>    :relids (b 1 2)
>
> to
>
>    :relids
>       {BITMAPSET
>        (b 1 2)
>       }
>
> or whatever the output would look like as the patch stands.
> So that needs a bit more effort, but it's surely manageable.

Agreed with leaving the dump format unchanged or not bloating it.

Thanks a lot for 5e1f3b9ebf6e5.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/CA+HiwqG8L3DVoZauJi1-eorLnnoM6VcfJCCauQX8=ofi-qMYCQ@mail.gmail.com
[2] https://www.postgresql.org/message-id/2901865.1667685211%40sss.pgh.pa.us