Unused function parameter in get_qual_from_partbound()
Hi,
I noticed that the first function parameter in get_qual_from_partbound(**Relation rel**, Relation parent,
is not used in the function.
Is it better to remove it like the attached patch ?
Best regards,
houzj
Attachments:
0001-remove-unused-function-parameter-in-get_qual_from_pa.patchapplication/octet-stream; name=0001-remove-unused-function-parameter-in-get_qual_from_pa.patchDownload
From 86a0772d2edfd5441cfd778db8c54153da032706 Mon Sep 17 00:00:00 2001
From: houzj <houzj.fnst@cn.fujitsu.com>
Date: Tue, 8 Jun 2021 17:45:32 +0800
Subject: [PATCH] remove unused function parameter in get_qual_from_partbound
---
src/backend/commands/tablecmds.c | 2 +-
src/backend/partitioning/partbounds.c | 3 +--
src/backend/utils/cache/partcache.c | 2 +-
src/include/partitioning/partbounds.h | 2 +-
4 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 028e8ac..722d43e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17247,7 +17247,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
* If the parent itself is a partition, make sure to include its
* constraint as well.
*/
- partBoundConstraint = get_qual_from_partbound(attachrel, rel, cmd->bound);
+ partBoundConstraint = get_qual_from_partbound(rel, cmd->bound);
partConstraint = list_concat(partBoundConstraint,
RelationGetPartitionQual(rel));
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcc..80294d3 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -246,8 +246,7 @@ static List *get_range_nulltest(PartitionKey key);
* expressions as partition constraint
*/
List *
-get_qual_from_partbound(Relation rel, Relation parent,
- PartitionBoundSpec *spec)
+get_qual_from_partbound(Relation parent, PartitionBoundSpec *spec)
{
PartitionKey key = RelationGetPartitionKey(parent);
List *my_qual = NIL;
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 21e60f0..f2b4867 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -376,7 +376,7 @@ generate_partition_qual(Relation rel)
bound = castNode(PartitionBoundSpec,
stringToNode(TextDatumGetCString(boundDatum)));
- my_qual = get_qual_from_partbound(rel, parent, bound);
+ my_qual = get_qual_from_partbound(parent, bound);
}
ReleaseSysCache(tuple);
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index ebf3ff1..2f00f9a 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -85,7 +85,7 @@ extern int get_hash_partition_greatest_modulus(PartitionBoundInfo b);
extern uint64 compute_partition_hash_value(int partnatts, FmgrInfo *partsupfunc,
Oid *partcollation,
Datum *values, bool *isnull);
-extern List *get_qual_from_partbound(Relation rel, Relation parent,
+extern List *get_qual_from_partbound(Relation parent,
PartitionBoundSpec *spec);
extern PartitionBoundInfo partition_bounds_create(PartitionBoundSpec **boundspecs,
int nparts, PartitionKey key, int **mapping);
--
2.7.2.windows.1
On Tue, 8 Jun 2021 at 21:50, houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
I noticed that the first function parameter in get_qual_from_partbound(**Relation rel**, Relation parent,
is not used in the function.Is it better to remove it like the attached patch ?
Going by [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/catalog/partition.c;h=6dab45f0edf8b1617d7239652fe36f113d30fd7a;hb=f0e44751d71 it was used when it went in. It looks like it was for
mapping attribute numbers between parent and partition rels.
Going by [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/catalog/partition.c;h=874e69d8d62e8e93164093e7352756ebfd0f69bc;hp=f54e1bdf3fb52cefed9b0d2fe7ab2a169231579d;hb=0563a3a8b5;hpb=0c2070cefa0e5d097b715c9a3b9b5499470019aa, it looks like it became unused due to the attribute
mapping code being moved down into map_partition_varattnos().
As for whether we should remove it or not, because it's an external
function that an extension might want to use, it would need to wait
until at least we branch for PG15.
Likely it's best to add the patch to the July commitfest so that we
can make a decision then.
David
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/catalog/partition.c;h=6dab45f0edf8b1617d7239652fe36f113d30fd7a;hb=f0e44751d71
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/backend/catalog/partition.c;h=874e69d8d62e8e93164093e7352756ebfd0f69bc;hp=f54e1bdf3fb52cefed9b0d2fe7ab2a169231579d;hb=0563a3a8b5;hpb=0c2070cefa0e5d097b715c9a3b9b5499470019aa
On Tuesday, June 8, 2021 7:30 PM David Rowley <dgrowleyml@gmail.com>
On Tue, 8 Jun 2021 at 21:50, houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
wrote:I noticed that the first function parameter in
get_qual_from_partbound(**Relation rel**, Relation parent, is not used in thefunction.
Is it better to remove it like the attached patch ?
Going by [1] it was used when it went in. It looks like it was for mapping attribute
numbers between parent and partition rels.Going by [2], it looks like it became unused due to the attribute mapping code
being moved down into map_partition_varattnos().As for whether we should remove it or not, because it's an external function
that an extension might want to use, it would need to wait until at least we
branch for PG15.Likely it's best to add the patch to the July commitfest so that we can make a
decision then.
OK, Thanks for the explanation.
Added to CF: https://commitfest.postgresql.org/33/3159/
Best regards,
houzj
On Wed, Jun 09, 2021 at 12:28:48AM +0000, houzj.fnst@fujitsu.com wrote:
OK, Thanks for the explanation.
Added to CF: https://commitfest.postgresql.org/33/3159/
At first glance, this looked to me like breaking something just for
sake of breaking it, but removing the rel argument could be helpful
to simplify any external code calling it as there would be no need for
this extra Relation. So that looks like a good idea, no need to rush
that into 14 though.
--
Michael
Hello,
Googling around, I didn't find any extensions that would break from this
change. Even if there are any, this change will simplify the relevant
callsites. It also aligns the interface nicely with get_qual_for_hash,
get_qual_for_list and get_qual_for_range.
Marking this as ready for committer. It can be committed when the branch
is cut for 15.
Regards,
Soumyadeep (VMware)
Marking this as ready for committer. It can be committed when the branch
is cut for 15.
I see that REL_14_STABLE is already cut. So this can go in now.
On Tue, Jun 8, 2021 at 10:50 PM Michael Paquier <michael@paquier.xyz> wrote:
At first glance, this looked to me like breaking something just for
sake of breaking it, but removing the rel argument could be helpful
to simplify any external code calling it as there would be no need for
this extra Relation. So that looks like a good idea, no need to rush
that into 14 though.
I found no external references in codesearch.debian.net. I plan to commit
this in the next couple of days unless there are objections.
--
John Naylor
EDB: http://www.enterprisedb.com
On Mon, Jul 12, 2021 at 8:46 AM John Naylor <john.naylor@enterprisedb.com>
wrote:
On Tue, Jun 8, 2021 at 10:50 PM Michael Paquier <michael@paquier.xyz>
wrote:
At first glance, this looked to me like breaking something just for
sake of breaking it, but removing the rel argument could be helpful
to simplify any external code calling it as there would be no need for
this extra Relation. So that looks like a good idea, no need to rush
that into 14 though.I found no external references in codesearch.debian.net. I plan to commit
this in the next couple of days unless there are objections.
This has been committed.
--
John Naylor
EDB: http://www.enterprisedb.com