Thread: pgsql: Refactor attribute mappings used in logical tuple conversion
Refactor attribute mappings used in logical tuple conversion Tuple conversion support in tupconvert.c is able to convert rowtypes between two relations, inner and outer, which are logically equivalent but have a different ordering or even dropped columns (used mainly for inheritance tree and partitions). This makes use of attribute mappings, which are simple arrays made of AttrNumber elements with a length matching the number of attributes of the outer relation. The length of the attribute mapping has been treated as completely independent of the mapping itself until now, making it easy to pass down an incorrect mapping length. This commit refactors the code related to attribute mappings and moves it into an independent facility called attmap.c, extracted from tupconvert.c. This merges the attribute mapping with its length, avoiding to try to guess what is the length of a mapping to use as this is computed once, when the map is built. This will avoid mistakes like what has been fixed in dc816e58, which has used an incorrect mapping length by matching it with the number of attributes of an inner relation (a child partition) instead of an outer relation (a partitioned table). Author: Michael Paquier Reviewed-by: Amit Langote Discussion: https://postgr.es/m/20191121042556.GD153437@paquier.xyz Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/e1551f96e643a52a035c3b35777d968bc073f7fc Modified Files -------------- src/backend/access/common/Makefile | 1 + src/backend/access/common/attmap.c | 320 +++++++++++++++++++++++++++++ src/backend/access/common/tupconvert.c | 287 ++------------------------ src/backend/catalog/index.c | 12 +- src/backend/catalog/partition.c | 11 +- src/backend/catalog/pg_constraint.c | 1 - src/backend/commands/indexcmds.c | 16 +- src/backend/commands/tablecmds.c | 98 +++++---- src/backend/executor/execMain.c | 22 +- src/backend/executor/execPartition.c | 57 +++-- src/backend/jit/llvm/llvmjit_expr.c | 1 - src/backend/parser/parse_utilcmd.c | 18 +- src/backend/replication/logical/relation.c | 10 +- src/backend/replication/logical/worker.c | 9 +- src/backend/rewrite/rewriteManip.c | 12 +- src/include/access/attmap.h | 52 +++++ src/include/access/tupconvert.h | 13 +- src/include/catalog/index.h | 2 +- src/include/parser/parse_utilcmd.h | 3 +- src/include/replication/logicalrelation.h | 3 +- src/include/rewrite/rewriteManip.h | 3 +- src/tools/pgindent/typedefs.list | 1 + 22 files changed, 533 insertions(+), 419 deletions(-)
On 2019-12-18 08:25, Michael Paquier wrote:
> Refactor attribute mappings used in logical tuple conversion
gcc (9.2.0) mutters this:
tupconvert.c: In function ‘execute_attr_map_tuple’:
tupconvert.c:146:8: warning: unused variable ‘outnatts’
[-Wunused-variable]
146 | int outnatts = map->outdesc->natts;
| ^~~~~~~~
(Otherwise compiles & runs fine.)
On Wed, Dec 18, 2019 at 08:43:47AM +0100, er wrote: > gcc (9.2.0) mutters this: > > tupconvert.c: In function ‘execute_attr_map_tuple’: > tupconvert.c:146:8: warning: unused variable ‘outnatts’ [-Wunused-variable] > 146 | int outnatts = map->outdesc->natts; > | ^~~~~~~~ > > (Otherwise compiles & runs fine.) This one could use a PG_USED_FOR_ASSERTS_ONLY to work properly, but it is more simple to just remove the variable. Thanks for the report, Erik. -- Michael
Attachment
On Wed, Dec 18, 2019 at 07:25:49AM +0000, Michael Paquier wrote: > Refactor attribute mappings used in logical tuple conversion > > Tuple conversion support in tupconvert.c is able to convert rowtypes > between two relations, inner and outer, which are logically equivalent > but have a different ordering or even dropped columns (used mainly for > inheritance tree and partitions). This makes use of attribute mappings, > which are simple arrays made of AttrNumber elements with a length > matching the number of attributes of the outer relation. The length of > the attribute mapping has been treated as completely independent of the > mapping itself until now, making it easy to pass down an incorrect > mapping length. Hmm. All buildfarm members are happy with this change, except prairiedog which is picky about the part in rewriteManip.h (dory is failing now but it passed once before with this commit, longfin is happy as well as my Mac laptop): In file included from index.c:66: ../../../src/include/rewrite/rewriteManip.h:20: error: redefinition of typedef 'AttrMap' ../../../src/include/access/attmap.h:38: error: previous declaration of 'AttrMap' was here An obvious fix would be to remove the declaration of AttrMap in rewriteManip.h to have an inclusion of attnum.h so as rewriteManip.h is still self-compilable, but I thought that we only wanted a dependency to parsenodes.h there. Any thoughts? -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> Hmm. All buildfarm members are happy with this change, except
> prairiedog which is picky about the part in rewriteManip.h
It's not only prairiedog --- my RHEL6 workstation (gcc 4.4.7) is failing
as well, and I see buildfarm member grouse is unhappy too.
> In file included from index.c:66:
> ../../../src/include/rewrite/rewriteManip.h:20: error: redefinition of
> typedef 'AttrMap'
This is simply the wrong way to do it. What you have to do, if you
want to not include attmap.h here, is to say
struct AttrMap;
(no typedef) and then refer to it as "struct AttrMap" in the rest
of the header file. There are lots of other examples in our headers.
BTW, it's also conventional to add a comment saying "this is to
avoid including foo.h", or equivalent.
TBH, though, I wonder if this doesn't indicate you've put this
function in the wrong header to begin with. Why does it belong
in rewriteManip?
regards, tom lane
On Wed, Dec 18, 2019 at 11:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > TBH, though, I wonder if this doesn't indicate you've put this > function in the wrong header to begin with. Why does it belong > in rewriteManip? Assuming you are talking about map_variable_attnos(), it's always been in rewriteManip.c / rewriteManip.h since it was added by 541ffa65c32. While reviewing this patch, I had the idea of moving it to the new header attmap.h, but thought it might be a good idea to keep attmap.c limited to just building the maps and not move into it other functions that do something useful with those maps, like translating expression trees, converting tuples, etc. Thanks, Amit
On Thu, Dec 19, 2019 at 10:23:32AM +0900, Amit Langote wrote: > On Wed, Dec 18, 2019 at 11:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> TBH, though, I wonder if this doesn't indicate you've put this >> function in the wrong header to begin with. Why does it belong >> in rewriteManip? Thanks for the fix! I was going to address that this morning to notice that you already committed a fix. > Assuming you are talking about map_variable_attnos(), it's always been > in rewriteManip.c / rewriteManip.h since it was added by 541ffa65c32. > > While reviewing this patch, I had the idea of moving it to the new > header attmap.h, but thought it might be a good idea to keep attmap.c > limited to just building the maps and not move into it other functions > that do something useful with those maps, like translating expression > trees, converting tuples, etc. I'd rather keep attmap.c focused on its basic work which is to make and build the attribute maps. For map_variable_attnos() & co, I am wondering if we should not split things even further. This code is located now in the rewriter, but we make use of it in the executor for partitioning. -- Michael