Re: Do we want a hashset type? - Mailing list pgsql-hackers

From jian he
Subject Re: Do we want a hashset type?
Date
Msg-id CACJufxEcnaTJV=vU1ftxEh7WaUMdbaeCpMQ2nhNVHx1q_QggDA@mail.gmail.com
Whole thread Raw
In response to Re: Do we want a hashset type?  ("Joel Jacobson" <joel@compiler.org>)
Responses Re: Do we want a hashset type?
List pgsql-hackers
On Thu, Jun 29, 2023 at 4:43 PM Joel Jacobson <joel@compiler.org> wrote:
>
> On Thu, Jun 29, 2023, at 08:54, jian he wrote:
> > Anyway, this time, I added another macro,which seems to simplify the code.
> >
> > #define SET_DATA_PTR(a) \
> > (((char *) (a->data)) + CEIL_DIV(a->capacity, 8))
> >
> > it passed all the tests on my local machine.
>
> Hmm, this is interesting. There is a bug in your second patch,
> that the tests catch, so it's really surprising if they pass on your machine.
>
> Can you try to run `make clean && make && make install && make installcheck`?
>
> I would guess you forgot to recompile or reinstall.
>
> This is the bug in 0002-marco-SET_DATA_PTR-to-quicly-access-hashset-data-reg.patch:
>
> @@ -411,7 +411,7 @@ int4hashset_union(PG_FUNCTION_ARGS)
>         int4hashset_t  *seta = PG_GETARG_INT4HASHSET_COPY(0);
>         int4hashset_t  *setb = PG_GETARG_INT4HASHSET(1);
>         char               *bitmap = setb->data;
> -       int32              *values = (int32 *) (bitmap + CEIL_DIV(setb->capacity, 8));
> +       int32              *values = (int32 *) SET_DATA_PTR(seta);
>
> You accidentally replaced `setb` with `seta`.
>
> I renamed the macro to HASHSET_GET_VALUES and changed it slightly,
> also added a HASHSET_GET_BITMAP for completeness:
>
> #define HASHSET_GET_BITMAP(set) ((set)->data)
> #define HASHSET_GET_VALUES(set) ((int32 *) ((set)->data + CEIL_DIV((set)->capacity, 8)))
>
> Instead of your version:
>
> #define SET_DATA_PTR(a) \
>                 (((char *) (a->data)) + CEIL_DIV(a->capacity, 8))
>
> Changes:
> * Parenthesize macro parameters.
> * Prefix the macro names with "HASHSET_" to avoid potential conflicts.
> * "GET_VALUES" more clearly communicates that it's the values we're extracting.
>
> New patch attached.
>
> Other changes in same commit:
>
> * Add original friends-of-friends graph query to new benchmark/ directory
> * Add table of content to README
> * Update docs: Explain null semantics and add function examples
> * Simplify empty hashset handling, remove unused includes
>
> /Joel

more like a C questions
in this context does
#define HASHSET_GET_VALUES(set) ((int32 *) ((set)->data +
CEIL_DIV((set)->capacity, 8)))
define first, then define struct int4hashset_t. Is this normally ok?

Also does
#define HASHSET_GET_VALUES(set) ((int32 *) ((set)->data +
CEIL_DIV((set)->capacity, 8)))

remove (int32 *) will make it generic? then when you use it, you can
cast whatever type you like?



pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: pg_decode_message vs skip_empty_xacts and xact_wrote_changes
Next
From: Nathan Bossart
Date:
Subject: Re: pgsql: Fix search_path to a safe value during maintenance operations.