Thread
Commits
-
Minor cleanup of the BRIN parallel build code
- 6c63bcbf3ccf 17.0 landed
-
Allow parallel CREATE INDEX for BRIN indexes
- b43757171470 17.0 cited
-
Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-26T18:10:30Z
Hi, The commit b437571 <http://b437571714707bc6466abde1a0af5e69aaade09c> I think has an oversight. When allocate memory and initialize private spool in function: _brin_leader_participate_as_worker The behavior is the bs_spool (heap and index fields) are left empty. The code affected is: buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); - buildstate->bs_spool->heap = buildstate->bs_spool->heap; - buildstate->bs_spool->index = buildstate->bs_spool->index; + buildstate->bs_spool->heap = heap; + buildstate->bs_spool->index = index; Is the fix correct? best regards, Ranier Vilela
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-12-26T22:07:09Z
On 12/26/23 19:10, Ranier Vilela wrote: > Hi, > > The commit b437571 <http://b437571714707bc6466abde1a0af5e69aaade09c> I > think has an oversight. > When allocate memory and initialize private spool in function: > _brin_leader_participate_as_worker > > The behavior is the bs_spool (heap and index fields) > are left empty. > > The code affected is: > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > - buildstate->bs_spool->index = buildstate->bs_spool->index; > + buildstate->bs_spool->heap = heap; > + buildstate->bs_spool->index = index; > > Is the fix correct? > Thanks for noticing this. Yes, I believe this is a bug - the assignments are certainly wrong, it leaves the fields set to NULL. I wonder how come this didn't fail during testing. Surely, if the leader participates as a worker, the tuplesort_begin_index_brin shall be called with heap/index being NULL, leading to some failure during the sort. But maybe this means we don't actually need the heap/index fields, it's just a copy of TuplesortIndexArg, but BRIN does not need that because we sort the tuples by blkno, and we don't need the descriptors for that. In any case, the _brin_parallel_scan_and_build does not actually need the separate heap/index arguments, those are already in the spool. I'll try to figure out if we want to simplify the tuplesort or remove the arguments from _brin_parallel_scan_and_build. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-27T11:37:25Z
Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra < tomas.vondra@enterprisedb.com> escreveu: > > > On 12/26/23 19:10, Ranier Vilela wrote: > > Hi, > > > > The commit b437571 <http://b437571714707bc6466abde1a0af5e69aaade09c> I > > think has an oversight. > > When allocate memory and initialize private spool in function: > > _brin_leader_participate_as_worker > > > > The behavior is the bs_spool (heap and index fields) > > are left empty. > > > > The code affected is: > > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > - buildstate->bs_spool->index = buildstate->bs_spool->index; > > + buildstate->bs_spool->heap = heap; > > + buildstate->bs_spool->index = index; > > > > Is the fix correct? > > > > Thanks for noticing this. You're welcome. > Yes, I believe this is a bug - the assignments > are certainly wrong, it leaves the fields set to NULL. > > I wonder how come this didn't fail during testing. Surely, if the leader > participates as a worker, the tuplesort_begin_index_brin shall be called > with heap/index being NULL, leading to some failure during the sort. But > maybe this means we don't actually need the heap/index fields, it's just > a copy of TuplesortIndexArg, but BRIN does not need that because we sort > the tuples by blkno, and we don't need the descriptors for that. > Unfortunately I can't test on Windows, since I can't build with meson on Windows. > In any case, the _brin_parallel_scan_and_build does not actually need > the separate heap/index arguments, those are already in the spool. > Yeah, for sure. > I'll try to figure out if we want to simplify the tuplesort or remove > the arguments from _brin_parallel_scan_and_build. > Thank you for your work. best regards, Ranier Vilela
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-12-29T01:16:08Z
On 12/27/23 12:37, Ranier Vilela wrote: > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> > escreveu: > > > > On 12/26/23 19:10, Ranier Vilela wrote: > > Hi, > > > > The commit b437571 > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c>> I > > think has an oversight. > > When allocate memory and initialize private spool in function: > > _brin_leader_participate_as_worker > > > > The behavior is the bs_spool (heap and index fields) > > are left empty. > > > > The code affected is: > > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > - buildstate->bs_spool->index = buildstate->bs_spool->index; > > + buildstate->bs_spool->heap = heap; > > + buildstate->bs_spool->index = index; > > > > Is the fix correct? > > > > Thanks for noticing this. > > You're welcome. > > > Yes, I believe this is a bug - the assignments > are certainly wrong, it leaves the fields set to NULL. > > I wonder how come this didn't fail during testing. Surely, if the leader > participates as a worker, the tuplesort_begin_index_brin shall be called > with heap/index being NULL, leading to some failure during the sort. But > maybe this means we don't actually need the heap/index fields, it's just > a copy of TuplesortIndexArg, but BRIN does not need that because we sort > the tuples by blkno, and we don't need the descriptors for that. > > Unfortunately I can't test on Windows, since I can't build with meson on > Windows. > > > In any case, the _brin_parallel_scan_and_build does not actually need > the separate heap/index arguments, those are already in the spool. > > Yeah, for sure. > > > I'll try to figure out if we want to simplify the tuplesort or remove > the arguments from _brin_parallel_scan_and_build. > Here is a patch simplifying the BRIN parallel create code a little bit. As I suspected, we don't need the heap/index in the spool at all, and we don't need to pass it to tuplesort_begin_index_brin either - we only need blkno, and we have that in the datum1 field. This also means we don't need TuplesortIndexBrinArg. I'll push this tomorrow, probably. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-29T11:53:44Z
Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra < tomas.vondra@enterprisedb.com> escreveu: > > > On 12/27/23 12:37, Ranier Vilela wrote: > > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra > > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> > > escreveu: > > > > > > > > On 12/26/23 19:10, Ranier Vilela wrote: > > > Hi, > > > > > > The commit b437571 > > <http://b437571714707bc6466abde1a0af5e69aaade09c > > <http://b437571714707bc6466abde1a0af5e69aaade09c>> I > > > think has an oversight. > > > When allocate memory and initialize private spool in function: > > > _brin_leader_participate_as_worker > > > > > > The behavior is the bs_spool (heap and index fields) > > > are left empty. > > > > > > The code affected is: > > > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); > > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > > - buildstate->bs_spool->index = buildstate->bs_spool->index; > > > + buildstate->bs_spool->heap = heap; > > > + buildstate->bs_spool->index = index; > > > > > > Is the fix correct? > > > > > > > Thanks for noticing this. > > > > You're welcome. > > > > > > Yes, I believe this is a bug - the assignments > > are certainly wrong, it leaves the fields set to NULL. > > > > I wonder how come this didn't fail during testing. Surely, if the > leader > > participates as a worker, the tuplesort_begin_index_brin shall be > called > > with heap/index being NULL, leading to some failure during the sort. > But > > maybe this means we don't actually need the heap/index fields, it's > just > > a copy of TuplesortIndexArg, but BRIN does not need that because we > sort > > the tuples by blkno, and we don't need the descriptors for that. > > > > Unfortunately I can't test on Windows, since I can't build with meson on > > Windows. > > > > > > In any case, the _brin_parallel_scan_and_build does not actually need > > the separate heap/index arguments, those are already in the spool. > > > > Yeah, for sure. > > > > > > I'll try to figure out if we want to simplify the tuplesort or remove > > the arguments from _brin_parallel_scan_and_build. > > > > Here is a patch simplifying the BRIN parallel create code a little bit. > As I suspected, we don't need the heap/index in the spool at all, and we > don't need to pass it to tuplesort_begin_index_brin either - we only > need blkno, and we have that in the datum1 field. This also means we > don't need TuplesortIndexBrinArg. > With Windows 10, msvc 2022, compile end pass ninja test. But, if you allow me, I would like to try another approach to simplification. Instead of increasing the arguments in the call, wouldn't it be better to decrease them and this way all arguments will be passed in the registers instead of on a stack? bs_spool may well contain this data and will probably be useful in the future. I made a v1 version, based on your patch, for your consideration. best regards, Ranier Vilela
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-12-29T13:33:07Z
On 12/29/23 12:53, Ranier Vilela wrote: > Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> > escreveu: > > > > On 12/27/23 12:37, Ranier Vilela wrote: > > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra > > <tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com> > <mailto:tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com>>> > > escreveu: > > > > > > > > On 12/26/23 19:10, Ranier Vilela wrote: > > > Hi, > > > > > > The commit b437571 > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c> > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c>>> I > > > think has an oversight. > > > When allocate memory and initialize private spool in function: > > > _brin_leader_participate_as_worker > > > > > > The behavior is the bs_spool (heap and index fields) > > > are left empty. > > > > > > The code affected is: > > > buildstate->bs_spool = (BrinSpool *) > palloc0(sizeof(BrinSpool)); > > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > > - buildstate->bs_spool->index = buildstate->bs_spool->index; > > > + buildstate->bs_spool->heap = heap; > > > + buildstate->bs_spool->index = index; > > > > > > Is the fix correct? > > > > > > > Thanks for noticing this. > > > > You're welcome. > > > > > > Yes, I believe this is a bug - the assignments > > are certainly wrong, it leaves the fields set to NULL. > > > > I wonder how come this didn't fail during testing. Surely, if > the leader > > participates as a worker, the tuplesort_begin_index_brin shall > be called > > with heap/index being NULL, leading to some failure during the > sort. But > > maybe this means we don't actually need the heap/index fields, > it's just > > a copy of TuplesortIndexArg, but BRIN does not need that > because we sort > > the tuples by blkno, and we don't need the descriptors for that. > > > > Unfortunately I can't test on Windows, since I can't build with > meson on > > Windows. > > > > > > In any case, the _brin_parallel_scan_and_build does not > actually need > > the separate heap/index arguments, those are already in the spool. > > > > Yeah, for sure. > > > > > > I'll try to figure out if we want to simplify the tuplesort or > remove > > the arguments from _brin_parallel_scan_and_build. > > > > Here is a patch simplifying the BRIN parallel create code a little bit. > As I suspected, we don't need the heap/index in the spool at all, and we > don't need to pass it to tuplesort_begin_index_brin either - we only > need blkno, and we have that in the datum1 field. This also means we > don't need TuplesortIndexBrinArg. > > With Windows 10, msvc 2022, compile end pass ninja test. > > But, if you allow me, I would like to try another approach to > simplification. > Instead of increasing the arguments in the call, wouldn't it be better > to decrease them > and this way all arguments will be passed in the registers instead of on > a stack? > If this was beneficial, we'd be passing everything through structs and not as explicit arguments. But we don't. If you're arguing it's beneficial in this case, it'd be good to see it demonstrated. > bs_spool may well contain this data and will probably be useful in the > future. > > I made a v1 version, based on your patch, for your consideration. > I did actually consider doing it this way yesterday, but I don't like this approach. I don't believe it's faster (and even if it was, the difference is going to be negligible), and parameters hidden in some struct increase the cognitive load. I like explicit arguments. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-29T13:53:12Z
Em sex., 29 de dez. de 2023 às 10:33, Tomas Vondra < tomas.vondra@enterprisedb.com> escreveu: > > > On 12/29/23 12:53, Ranier Vilela wrote: > > Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra > > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> > > escreveu: > > > > > > > > On 12/27/23 12:37, Ranier Vilela wrote: > > > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra > > > <tomas.vondra@enterprisedb.com > > <mailto:tomas.vondra@enterprisedb.com> > > <mailto:tomas.vondra@enterprisedb.com > > <mailto:tomas.vondra@enterprisedb.com>>> > > > escreveu: > > > > > > > > > > > > On 12/26/23 19:10, Ranier Vilela wrote: > > > > Hi, > > > > > > > > The commit b437571 > > > <http://b437571714707bc6466abde1a0af5e69aaade09c > > <http://b437571714707bc6466abde1a0af5e69aaade09c> > > > <http://b437571714707bc6466abde1a0af5e69aaade09c > > <http://b437571714707bc6466abde1a0af5e69aaade09c>>> I > > > > think has an oversight. > > > > When allocate memory and initialize private spool in > function: > > > > _brin_leader_participate_as_worker > > > > > > > > The behavior is the bs_spool (heap and index fields) > > > > are left empty. > > > > > > > > The code affected is: > > > > buildstate->bs_spool = (BrinSpool *) > > palloc0(sizeof(BrinSpool)); > > > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > > > - buildstate->bs_spool->index = buildstate->bs_spool->index; > > > > + buildstate->bs_spool->heap = heap; > > > > + buildstate->bs_spool->index = index; > > > > > > > > Is the fix correct? > > > > > > > > > > Thanks for noticing this. > > > > > > You're welcome. > > > > > > > > > Yes, I believe this is a bug - the assignments > > > are certainly wrong, it leaves the fields set to NULL. > > > > > > I wonder how come this didn't fail during testing. Surely, if > > the leader > > > participates as a worker, the tuplesort_begin_index_brin shall > > be called > > > with heap/index being NULL, leading to some failure during the > > sort. But > > > maybe this means we don't actually need the heap/index fields, > > it's just > > > a copy of TuplesortIndexArg, but BRIN does not need that > > because we sort > > > the tuples by blkno, and we don't need the descriptors for > that. > > > > > > Unfortunately I can't test on Windows, since I can't build with > > meson on > > > Windows. > > > > > > > > > In any case, the _brin_parallel_scan_and_build does not > > actually need > > > the separate heap/index arguments, those are already in the > spool. > > > > > > Yeah, for sure. > > > > > > > > > I'll try to figure out if we want to simplify the tuplesort or > > remove > > > the arguments from _brin_parallel_scan_and_build. > > > > > > > Here is a patch simplifying the BRIN parallel create code a little > bit. > > As I suspected, we don't need the heap/index in the spool at all, > and we > > don't need to pass it to tuplesort_begin_index_brin either - we only > > need blkno, and we have that in the datum1 field. This also means we > > don't need TuplesortIndexBrinArg. > > > > With Windows 10, msvc 2022, compile end pass ninja test. > > > > But, if you allow me, I would like to try another approach to > > simplification. > > Instead of increasing the arguments in the call, wouldn't it be better > > to decrease them > > and this way all arguments will be passed in the registers instead of on > > a stack? > > > > If this was beneficial, we'd be passing everything through structs and > not as explicit arguments. But we don't. If you're arguing it's > beneficial in this case, it'd be good to see it demonstrated. > Please see the https://www.agner.org/optimize/optimizing_cpp.pdf Excerpt: "Use 64-bit mode Parameter transfer is more efficient in 64-bit mode than in 32-bit mode, and more efficient in 64-bit Linux than in 64-bit Windows. In 64-bit Linux, the first six integer parameters and the first eight floating point parameters are transferred in registers, totaling up to fourteen register parameters. In 64-bit Windows, the first four parameters are transferred in registers, regardless of whether they are integers or floating point numbers." With function: _brin_parallel_scan_and_build(buildstate, buildstate->bs_spool, brinshared, sharedsort, heapRel, indexRel, sortmem, false); We have: Linux -> six first parameters in registers and two parameters in stack Windows -> four parameters in registers and four parameters in stack > > bs_spool may well contain this data and will probably be useful in the > > future. > > > > I made a v1 version, based on your patch, for your consideration. > > > > I did actually consider doing it this way yesterday, but I don't like > this approach. I don't believe it's faster (and even if it was, the > difference is going to be negligible), and parameters hidden in some > struct increase the cognitive load. I like explicit arguments. > Personally I prefer data in structs, of course, always thinking about size and alignment, to optimize loading. Best regards, Ranier Vilela
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-12-29T14:32:18Z
On 12/29/23 14:53, Ranier Vilela wrote: > > > Em sex., 29 de dez. de 2023 às 10:33, Tomas Vondra > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> > escreveu: > > > > On 12/29/23 12:53, Ranier Vilela wrote: > > Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra > > <tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com> > <mailto:tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com>>> > > escreveu: > > > > > > > > On 12/27/23 12:37, Ranier Vilela wrote: > > > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra > > > <tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com> > > <mailto:tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com>> > > <mailto:tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com> > > <mailto:tomas.vondra@enterprisedb.com > <mailto:tomas.vondra@enterprisedb.com>>>> > > > escreveu: > > > > > > > > > > > > On 12/26/23 19:10, Ranier Vilela wrote: > > > > Hi, > > > > > > > > The commit b437571 > > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c> > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c>> > > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c> > > <http://b437571714707bc6466abde1a0af5e69aaade09c > <http://b437571714707bc6466abde1a0af5e69aaade09c>>>> I > > > > think has an oversight. > > > > When allocate memory and initialize private spool in > function: > > > > _brin_leader_participate_as_worker > > > > > > > > The behavior is the bs_spool (heap and index fields) > > > > are left empty. > > > > > > > > The code affected is: > > > > buildstate->bs_spool = (BrinSpool *) > > palloc0(sizeof(BrinSpool)); > > > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; > > > > - buildstate->bs_spool->index = > buildstate->bs_spool->index; > > > > + buildstate->bs_spool->heap = heap; > > > > + buildstate->bs_spool->index = index; > > > > > > > > Is the fix correct? > > > > > > > > > > Thanks for noticing this. > > > > > > You're welcome. > > > > > > > > > Yes, I believe this is a bug - the assignments > > > are certainly wrong, it leaves the fields set to NULL. > > > > > > I wonder how come this didn't fail during testing. > Surely, if > > the leader > > > participates as a worker, the tuplesort_begin_index_brin > shall > > be called > > > with heap/index being NULL, leading to some failure > during the > > sort. But > > > maybe this means we don't actually need the heap/index > fields, > > it's just > > > a copy of TuplesortIndexArg, but BRIN does not need that > > because we sort > > > the tuples by blkno, and we don't need the descriptors > for that. > > > > > > Unfortunately I can't test on Windows, since I can't build with > > meson on > > > Windows. > > > > > > > > > In any case, the _brin_parallel_scan_and_build does not > > actually need > > > the separate heap/index arguments, those are already in > the spool. > > > > > > Yeah, for sure. > > > > > > > > > I'll try to figure out if we want to simplify the > tuplesort or > > remove > > > the arguments from _brin_parallel_scan_and_build. > > > > > > > Here is a patch simplifying the BRIN parallel create code a > little bit. > > As I suspected, we don't need the heap/index in the spool at > all, and we > > don't need to pass it to tuplesort_begin_index_brin either - > we only > > need blkno, and we have that in the datum1 field. This also > means we > > don't need TuplesortIndexBrinArg. > > > > With Windows 10, msvc 2022, compile end pass ninja test. > > > > But, if you allow me, I would like to try another approach to > > simplification. > > Instead of increasing the arguments in the call, wouldn't it be better > > to decrease them > > and this way all arguments will be passed in the registers instead > of on > > a stack? > > > > If this was beneficial, we'd be passing everything through structs and > not as explicit arguments. But we don't. If you're arguing it's > beneficial in this case, it'd be good to see it demonstrated. > > Please see the https://www.agner.org/optimize/optimizing_cpp.pdf > <https://www.agner.org/optimize/optimizing_cpp.pdf> > Excerpt: > "Use 64-bit mode > Parameter transfer is more efficient in 64-bit mode than in 32-bit mode, > and more efficient in 64-bit Linux than in 64-bit Windows. In 64-bit > Linux, the first six integer parameters and the first eight floating > point parameters are transferred in registers, totaling up to fourteen > register parameters. In 64-bit Windows, the first four parameters are > transferred in registers, regardless of whether they are integers or > floating point numbers." > > With function: > _brin_parallel_scan_and_build(buildstate, buildstate->bs_spool, > brinshared, sharedsort, heapRel, indexRel, sortmem, false); > We have: > Linux -> six first parameters in registers and two parameters in stack > Windows -> four parameters in registers and four parameters in stack > I suggested you demonstrate this actually makes a difference in practice. Quoting a document is not that. Also, that document is about C++, and while C and C++ are very close, I wouldn't be surprised if there were differences. Furthermore, that section talks about integer/floating point arguments, while we're dealing with pointers, and it's not clear if that changes something (the document has a separate section about pointers/references, which suggests pointers and integers are not 100% the same thing). And finally, I haven't tried disassembling the code, but I'd be quite surprised if these things were not heavily dependent on the compiler and/or optimization level. > > > bs_spool may well contain this data and will probably be useful in the > > future. > > > > I made a v1 version, based on your patch, for your consideration. > > > > I did actually consider doing it this way yesterday, but I don't like > this approach. I don't believe it's faster (and even if it was, the > difference is going to be negligible), and parameters hidden in some > struct increase the cognitive load. I like explicit arguments. > > Personally I prefer data in structs, of course, > always thinking about size and alignment, to optimize loading. > As I said, I think this is quite irrelevant because we'll call the function maybe 10-times during the whole index build. With millions of other function calls. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-29T17:02:11Z
Em sex., 29 de dez. de 2023 às 08:53, Ranier Vilela <ranier.vf@gmail.com> escreveu: > Em qui., 28 de dez. de 2023 às 22:16, Tomas Vondra < > tomas.vondra@enterprisedb.com> escreveu: > >> >> >> On 12/27/23 12:37, Ranier Vilela wrote: >> > Em ter., 26 de dez. de 2023 às 19:07, Tomas Vondra >> > <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>> >> > escreveu: >> > >> > >> > >> > On 12/26/23 19:10, Ranier Vilela wrote: >> > > Hi, >> > > >> > > The commit b437571 >> > <http://b437571714707bc6466abde1a0af5e69aaade09c >> > <http://b437571714707bc6466abde1a0af5e69aaade09c>> I >> > > think has an oversight. >> > > When allocate memory and initialize private spool in function: >> > > _brin_leader_participate_as_worker >> > > >> > > The behavior is the bs_spool (heap and index fields) >> > > are left empty. >> > > >> > > The code affected is: >> > > buildstate->bs_spool = (BrinSpool *) palloc0(sizeof(BrinSpool)); >> > > - buildstate->bs_spool->heap = buildstate->bs_spool->heap; >> > > - buildstate->bs_spool->index = buildstate->bs_spool->index; >> > > + buildstate->bs_spool->heap = heap; >> > > + buildstate->bs_spool->index = index; >> > > >> > > Is the fix correct? >> > > >> > >> > Thanks for noticing this. >> > >> > You're welcome. >> > >> > >> > Yes, I believe this is a bug - the assignments >> > are certainly wrong, it leaves the fields set to NULL. >> > >> > I wonder how come this didn't fail during testing. Surely, if the >> leader >> > participates as a worker, the tuplesort_begin_index_brin shall be >> called >> > with heap/index being NULL, leading to some failure during the >> sort. But >> > maybe this means we don't actually need the heap/index fields, it's >> just >> > a copy of TuplesortIndexArg, but BRIN does not need that because we >> sort >> > the tuples by blkno, and we don't need the descriptors for that. >> > >> > Unfortunately I can't test on Windows, since I can't build with meson on >> > Windows. >> > >> > >> > In any case, the _brin_parallel_scan_and_build does not actually >> need >> > the separate heap/index arguments, those are already in the spool. >> > >> > Yeah, for sure. >> > >> > >> > I'll try to figure out if we want to simplify the tuplesort or >> remove >> > the arguments from _brin_parallel_scan_and_build. >> > >> >> Here is a patch simplifying the BRIN parallel create code a little bit. >> As I suspected, we don't need the heap/index in the spool at all, and we >> don't need to pass it to tuplesort_begin_index_brin either - we only >> need blkno, and we have that in the datum1 field. This also means we >> don't need TuplesortIndexBrinArg. >> > With Windows 10, msvc 2022, compile end pass ninja test. > > But, if you allow me, I would like to try another approach to > simplification. > Instead of increasing the arguments in the call, wouldn't it be better to > decrease them > and this way all arguments will be passed in the registers instead of on a > stack? > > bs_spool may well contain this data and will probably be useful in the > future. > > I made a v1 version, based on your patch, for your consideration. > As I wrote, the new patch version was for consideration. It seems more like a question of style, so it's better to remove it. Anyway +1 for your original patch. Best regards, Ranier Vilela
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Tomas Vondra <tomas.vondra@enterprisedb.com> — 2023-12-30T22:19:38Z
On 12/29/23 18:02, Ranier Vilela wrote: > > ... > > As I wrote, the new patch version was for consideration. > It seems more like a question of style, so it's better to remove it. > > Anyway +1 for your original patch. > I've pushed my original patch. Thanks for the report. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: Fix Brin Private Spool Initialization (src/backend/access/brin/brin.c)
Ranier Vilela <ranier.vf@gmail.com> — 2023-12-30T22:50:05Z
Em sáb., 30 de dez. de 2023 19:19, Tomas Vondra < tomas.vondra@enterprisedb.com> escreveu: > On 12/29/23 18:02, Ranier Vilela wrote: > > > > ... > > > > As I wrote, the new patch version was for consideration. > > It seems more like a question of style, so it's better to remove it. > > > > Anyway +1 for your original patch. > > > > I've pushed my original patch. Thanks for the report. > Thank you. Best regards, Ranier Vilela