On Sat, Nov 12, 2022 at 1:46 AM Tom Lane <[email protected]> wrote:
> Alvaro Herrera <[email protected]> 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/[email protected]
>
> 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-qm...@mail.gmail.com
[2] https://www.postgresql.org/message-id/2901865.1667685211%40sss.pgh.pa.us
