public inbox for [email protected]  
help / color / mirror / Atom feed
Extend compatibility of PostgreSQL::Test::Cluster
10+ messages / 5 participants
[nested] [flat]

* Extend compatibility of PostgreSQL::Test::Cluster
@ 2021-12-28 14:30  Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Andrew Dunstan @ 2021-12-28 14:30 UTC (permalink / raw)
  To: PostgreSQL Hackers <[email protected]>


PFA a patch to extend the compatibility of PostgreSQL::Test::Cluster to
all live branches. It does this by introducing a couple of subclasses
which override a few things. The required class is automatically
detected and used, so users don't need to specify a subclass. Although
this is my work it draws some inspiration from work by Jehan-Guillaume
de Rorthais. The aim here is to provide minimal disruption to the
mainline code, and also to have very small override subroutines.

My hope is to take this further, down to 9.2, which we recently decided
to give limited build support to. However I think the present patch is a
good stake to put into the ground.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com


Attachments:

  [text/x-patch] cluster-compat-extension.patch (3.8K, ../../[email protected]/2-cluster-compat-extension.patch)
  download | inline diff:
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index c061e850fb..0e0bf0ecfc 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -111,6 +111,10 @@ use Scalar::Util qw(blessed);
 our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
 	$last_port_assigned, @all_nodes, $died);
 
+# the minimum version we believe to be compatible with this package without
+# subclassing.
+our $min_compat = 12;
+
 INIT
 {
 
@@ -1018,7 +1022,7 @@ sub enable_streaming
 
 	print "### Enabling streaming replication for node \"$name\"\n";
 	$self->append_conf(
-		'postgresql.conf', qq(
+		$self->_recovery_file, qq(
 primary_conninfo='$root_connstr'
 ));
 	$self->set_standby_mode();
@@ -1047,7 +1051,7 @@ sub enable_restoring
 	  : qq{cp "$path/%f" "%p"};
 
 	$self->append_conf(
-		'postgresql.conf', qq(
+		$self->_recovery_file, qq(
 restore_command = '$copy_command'
 ));
 	if ($standby)
@@ -1061,6 +1065,8 @@ restore_command = '$copy_command'
 	return;
 }
 
+sub _recovery_file { return "postgresql.conf"; }
+
 =pod
 
 =item $node->set_recovery_mode()
@@ -1246,15 +1252,24 @@ sub new
 
 	$node->dump_info;
 
-	# Add node to list of nodes
-	push(@all_nodes, $node);
-
 	$node->_set_pg_version;
 
-	my $v = $node->{_pg_version};
+	my $ver = $node->{_pg_version};
 
-	carp("PostgreSQL::Test::Cluster isn't fully compatible with version " . $v)
-	  if $v < 12;
+	# Use a subclass as defined below (or elsewhere) if this version
+	# isn't fully compatible. If the subclass doesn't exist then bad things
+	# will happen when you try to use the node. However, there isn't a simple
+	# test to see if the class exists, and we don't want to create another
+	# instance in order to check.
+	if (ref $ver && $ver < $min_compat)
+    {
+        my $maj      = $ver->major(separator => '_');
+        my $subclass = __PACKAGE__ . "::V_$maj";
+        bless $node, $subclass;
+    }
+
+	# Add node to list of nodes
+	push(@all_nodes, $node);
 
 	return $node;
 }
@@ -2546,8 +2561,12 @@ sub wait_for_catchup
 	  . "_lsn to pass "
 	  . $lsn_expr . " on "
 	  . $self->name . "\n";
+    # old versions of walreceiver just set the application name to
+    # `walreceiver'
 	my $query =
-	  qq[SELECT $lsn_expr <= ${mode}_lsn AND state = 'streaming' FROM pg_catalog.pg_stat_replication WHERE application_name = '$standby_name';];
+	  qq[SELECT $lsn_expr <= ${mode}_lsn AND state = 'streaming'
+         FROM pg_catalog.pg_stat_replication
+         WHERE application_name in ('$standby_name','walreceiver');];
 	$self->poll_query_until('postgres', $query)
 	  or croak "timed out waiting for catchup";
 	print "done\n";
@@ -2771,4 +2790,41 @@ sub pg_recvlogical_upto
 
 =cut
 
+##########################################################################
+
+package PostgreSQL::Test::Cluster::V_11; ## no critic (ProhibitMultiplePackages)
+
+use parent -norequire, qw(PostgreSQL::Test::Cluster);
+
+# https://www.postgresql.org/docs/11/release-11.html
+
+# max_wal_senders + superuser_reserved_connections must be < max_connections
+# uses recovery.conf
+
+sub _recovery_file { return "recovery.conf"; }
+
+sub set_standby_mode
+{
+    my $self = shift;
+    $self->append_conf("recovery.conf", "standby_mode = on\n");
+}
+
+sub init
+{
+    my ($self, %params) = @_;
+    $self->SUPER::init(%params);
+    $self->adjust_conf('postgresql.conf', 'max_wal_senders',
+                      $params{allows_streaming} ? 5 : 0);
+}
+
+##########################################################################
+
+package PostgreSQL::Test::Cluster::V_10; ## no critic (ProhibitMultiplePackages)
+
+use parent -norequire, qw(PostgreSQL::Test::Cluster::V_11);
+
+# https://www.postgresql.org/docs/10/release-10.html
+
+########################################################################
+
 1;


^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2021-12-28 16:46  Andrew Dunstan <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Andrew Dunstan @ 2021-12-28 16:46 UTC (permalink / raw)
  To: PostgreSQL Hackers <[email protected]>


On 12/28/21 09:30, Andrew Dunstan wrote:
> PFA a patch to extend the compatibility of PostgreSQL::Test::Cluster to
> all live branches. It does this by introducing a couple of subclasses
> which override a few things. The required class is automatically
> detected and used, so users don't need to specify a subclass. Although
> this is my work it draws some inspiration from work by Jehan-Guillaume
> de Rorthais. The aim here is to provide minimal disruption to the
> mainline code, and also to have very small override subroutines.
>
> My hope is to take this further, down to 9.2, which we recently decided
> to give limited build support to. However I think the present patch is a
> good stake to put into the ground.



This version handles older versions for which we have no subclass more
gracefully.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com


Attachments:

  [text/x-patch] cluster-compat-extension-v2.patch (3.8K, ../../[email protected]/2-cluster-compat-extension-v2.patch)
  download | inline diff:
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index c061e850fb..89504128ec 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -111,6 +111,10 @@ use Scalar::Util qw(blessed);
 our ($use_tcp, $test_localhost, $test_pghost, $last_host_assigned,
 	$last_port_assigned, @all_nodes, $died);
 
+# the minimum version we believe to be compatible with this package without
+# subclassing.
+our $min_compat = 12;
+
 INIT
 {
 
@@ -1018,7 +1022,7 @@ sub enable_streaming
 
 	print "### Enabling streaming replication for node \"$name\"\n";
 	$self->append_conf(
-		'postgresql.conf', qq(
+		$self->_recovery_file, qq(
 primary_conninfo='$root_connstr'
 ));
 	$self->set_standby_mode();
@@ -1047,7 +1051,7 @@ sub enable_restoring
 	  : qq{cp "$path/%f" "%p"};
 
 	$self->append_conf(
-		'postgresql.conf', qq(
+		$self->_recovery_file, qq(
 restore_command = '$copy_command'
 ));
 	if ($standby)
@@ -1061,6 +1065,8 @@ restore_command = '$copy_command'
 	return;
 }
 
+sub _recovery_file { return "postgresql.conf"; }
+
 =pod
 
 =item $node->set_recovery_mode()
@@ -1246,15 +1252,28 @@ sub new
 
 	$node->dump_info;
 
-	# Add node to list of nodes
-	push(@all_nodes, $node);
-
 	$node->_set_pg_version;
 
-	my $v = $node->{_pg_version};
+	my $ver = $node->{_pg_version};
+
+	# Use a subclass as defined below (or elsewhere) if this version
+	# isn't fully compatible. Warn if the version is too old and thus we don't
+	# have a subclass of this class.
+	if (ref $ver && $ver < $min_compat)
+    {
+		my $maj      = $ver->major(separator => '_');
+		my $subclass = __PACKAGE__ . "::V_$maj";
+		bless $node, $subclass;
+		unless ($node->isa(__PACKAGE__))
+		{
+			# It's not a subclass, so re-bless back into the main package
+			bless($node, __PACKAGE__);
+			carp "PostgreSQL::Test::Cluster isn't fully compatible with version $ver";
+		}
+    }
 
-	carp("PostgreSQL::Test::Cluster isn't fully compatible with version " . $v)
-	  if $v < 12;
+	# Add node to list of nodes
+	push(@all_nodes, $node);
 
 	return $node;
 }
@@ -2546,8 +2565,12 @@ sub wait_for_catchup
 	  . "_lsn to pass "
 	  . $lsn_expr . " on "
 	  . $self->name . "\n";
+    # old versions of walreceiver just set the application name to
+    # `walreceiver'
 	my $query =
-	  qq[SELECT $lsn_expr <= ${mode}_lsn AND state = 'streaming' FROM pg_catalog.pg_stat_replication WHERE application_name = '$standby_name';];
+	  qq[SELECT $lsn_expr <= ${mode}_lsn AND state = 'streaming'
+         FROM pg_catalog.pg_stat_replication
+         WHERE application_name in ('$standby_name','walreceiver');];
 	$self->poll_query_until('postgres', $query)
 	  or croak "timed out waiting for catchup";
 	print "done\n";
@@ -2771,4 +2794,41 @@ sub pg_recvlogical_upto
 
 =cut
 
+##########################################################################
+
+package PostgreSQL::Test::Cluster::V_11; ## no critic (ProhibitMultiplePackages)
+
+use parent -norequire, qw(PostgreSQL::Test::Cluster);
+
+# https://www.postgresql.org/docs/11/release-11.html
+
+# max_wal_senders + superuser_reserved_connections must be < max_connections
+# uses recovery.conf
+
+sub _recovery_file { return "recovery.conf"; }
+
+sub set_standby_mode
+{
+    my $self = shift;
+    $self->append_conf("recovery.conf", "standby_mode = on\n");
+}
+
+sub init
+{
+    my ($self, %params) = @_;
+    $self->SUPER::init(%params);
+    $self->adjust_conf('postgresql.conf', 'max_wal_senders',
+                      $params{allows_streaming} ? 5 : 0);
+}
+
+##########################################################################
+
+package PostgreSQL::Test::Cluster::V_10; ## no critic (ProhibitMultiplePackages)
+
+use parent -norequire, qw(PostgreSQL::Test::Cluster::V_11);
+
+# https://www.postgresql.org/docs/10/release-10.html
+
+########################################################################
+
 1;


^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2021-12-31 16:20  Dagfinn Ilmari Mannsåker <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Dagfinn Ilmari Mannsåker @ 2021-12-31 16:20 UTC (permalink / raw)
  To: Andrew Dunstan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>

Andrew Dunstan <[email protected]> writes:

> +		my $subclass = __PACKAGE__ . "::V_$maj";
> +		bless $node, $subclass;
> +		unless ($node->isa(__PACKAGE__))
> +		{
> +			# It's not a subclass, so re-bless back into the main package
> +			bless($node, __PACKAGE__);
> +			carp "PostgreSQL::Test::Cluster isn't fully compatible with version $ver";
> +		}

The ->isa() method works on package names as well as blessed objects, so
the back-and-forth blessing can be avoided.

	my $subclass = __PACKAGE__ . "::V_$maj";
	if ($subclass->isa(__PACKAGE__))
	{
		bless($node, $subclass);
	}
	else
	{
		carp "PostgreSQL::Test::Cluster isn't fully compatible with version $ver";
	}

- ilmari





^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2021-12-31 16:22  Andrew Dunstan <[email protected]>
  parent: Dagfinn Ilmari Mannsåker <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Andrew Dunstan @ 2021-12-31 16:22 UTC (permalink / raw)
  To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>


On 12/31/21 11:20, Dagfinn Ilmari Mannsåker wrote:
> Andrew Dunstan <[email protected]> writes:
>
>> +		my $subclass = __PACKAGE__ . "::V_$maj";
>> +		bless $node, $subclass;
>> +		unless ($node->isa(__PACKAGE__))
>> +		{
>> +			# It's not a subclass, so re-bless back into the main package
>> +			bless($node, __PACKAGE__);
>> +			carp "PostgreSQL::Test::Cluster isn't fully compatible with version $ver";
>> +		}
> The ->isa() method works on package names as well as blessed objects, so
> the back-and-forth blessing can be avoided.
>
> 	my $subclass = __PACKAGE__ . "::V_$maj";
> 	if ($subclass->isa(__PACKAGE__))
> 	{
> 		bless($node, $subclass);
> 	}
> 	else
> 	{
> 		carp "PostgreSQL::Test::Cluster isn't fully compatible with version $ver";
> 	}
>

OK, thanks, will fix in next version.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com






^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2022-03-29 21:56  Andrew Dunstan <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Andrew Dunstan @ 2022-03-29 21:56 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; PostgreSQL Hackers <[email protected]>


On 1/21/22 09:59, Andrew Dunstan wrote:
> On 1/21/22 02:47, Michael Paquier wrote:
>> On Tue, Jan 18, 2022 at 06:35:39PM -0500, Andrew Dunstan wrote:
>>> Here's a version that does that and removes some recent bitrot.
>> I have been looking at the full set of features of Cluster.pm and the
>> requirements behind v10 as minimal version supported, and nothing
>> really stands out.
>>
>> +   # old versions of walreceiver just set the application name to
>> +   # `walreceiver'
>>
>> Perhaps this should mention to which older versions this sentence
>> applies?
>
>
> Will do in the next version. FTR it's versions older than 12.
>
>

I'm not sure why this item has been moved to the next CF without any
discussion I could see on the mailing list. It was always my intention
to commit it this time, and I propose to do so tomorrow with the comment
Michael has requested above. The cfbot is still happy with it.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com






^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2022-03-30 05:55  Michael Paquier <[email protected]>
  parent: Andrew Dunstan <[email protected]>
  0 siblings, 1 reply; 10+ messages in thread

From: Michael Paquier @ 2022-03-30 05:55 UTC (permalink / raw)
  To: Andrew Dunstan <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; PostgreSQL Hackers <[email protected]>

On Tue, Mar 29, 2022 at 05:56:02PM -0400, Andrew Dunstan wrote:
> I'm not sure why this item has been moved to the next CF without any
> discussion I could see on the mailing list. It was always my intention
> to commit it this time, and I propose to do so tomorrow with the comment
> Michael has requested above. The cfbot is still happy with it.

Thanks for taking care of it!
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Extend compatibility of PostgreSQL::Test::Cluster
@ 2022-03-30 15:27  Andrew Dunstan <[email protected]>
  parent: Michael Paquier <[email protected]>
  0 siblings, 0 replies; 10+ messages in thread

From: Andrew Dunstan @ 2022-03-30 15:27 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; PostgreSQL Hackers <[email protected]>


On 3/30/22 01:55, Michael Paquier wrote:
> On Tue, Mar 29, 2022 at 05:56:02PM -0400, Andrew Dunstan wrote:
>> I'm not sure why this item has been moved to the next CF without any
>> discussion I could see on the mailing list. It was always my intention
>> to commit it this time, and I propose to do so tomorrow with the comment
>> Michael has requested above. The cfbot is still happy with it.
> Thanks for taking care of it!



Committed.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com






^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* [PATCH v3 1/4] Fix error handling in libpqrcv_connect()
@ 2023-01-21 02:27  Andres Freund <[email protected]>
  0 siblings, 0 replies; 10+ messages in thread

From: Andres Freund @ 2023-01-21 02:27 UTC (permalink / raw)

Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
 .../libpqwalreceiver/libpqwalreceiver.c       | 26 +++++++++++--------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index c40c6220db8..fefc8660259 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -175,10 +175,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	conn->streamConn = PQconnectStartParams(keys, vals,
 											 /* expand_dbname = */ true);
 	if (PQstatus(conn->streamConn) == CONNECTION_BAD)
-	{
-		*err = pchomp(PQerrorMessage(conn->streamConn));
-		return NULL;
-	}
+		goto bad_connection_errmsg;
 
 	/*
 	 * Poll connection until we have OK or FAILED status.
@@ -220,10 +217,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	} while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED);
 
 	if (PQstatus(conn->streamConn) != CONNECTION_OK)
-	{
-		*err = pchomp(PQerrorMessage(conn->streamConn));
-		return NULL;
-	}
+		goto bad_connection_errmsg;
 
 	if (logical)
 	{
@@ -234,9 +228,9 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 		if (PQresultStatus(res) != PGRES_TUPLES_OK)
 		{
 			PQclear(res);
-			ereport(ERROR,
-					(errmsg("could not clear search path: %s",
-							pchomp(PQerrorMessage(conn->streamConn)))));
+			*err = psprintf(_("could not clear search path: %s"),
+							pchomp(PQerrorMessage(conn->streamConn)));
+			goto bad_connection;
 		}
 		PQclear(res);
 	}
@@ -244,6 +238,16 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	conn->logical = logical;
 
 	return conn;
+
+	/* error path, using libpq's error message */
+bad_connection_errmsg:
+	*err = pchomp(PQerrorMessage(conn->streamConn));
+
+	/* error path, error already set */
+bad_connection:
+	PQfinish(conn->streamConn);
+	pfree(conn);
+	return NULL;
 }
 
 /*
-- 
2.38.0


--ivo3oksshniggdzf
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0002-Add-helper-library-for-use-of-libpq-inside-the-se.patch"



^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* [PATCH v3 1/4] Fix error handling in libpqrcv_connect()
@ 2023-01-21 02:27  Andres Freund <[email protected]>
  0 siblings, 0 replies; 10+ messages in thread

From: Andres Freund @ 2023-01-21 02:27 UTC (permalink / raw)

Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
 .../libpqwalreceiver/libpqwalreceiver.c       | 26 +++++++++++--------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index c40c6220db8..fefc8660259 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -175,10 +175,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	conn->streamConn = PQconnectStartParams(keys, vals,
 											 /* expand_dbname = */ true);
 	if (PQstatus(conn->streamConn) == CONNECTION_BAD)
-	{
-		*err = pchomp(PQerrorMessage(conn->streamConn));
-		return NULL;
-	}
+		goto bad_connection_errmsg;
 
 	/*
 	 * Poll connection until we have OK or FAILED status.
@@ -220,10 +217,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	} while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED);
 
 	if (PQstatus(conn->streamConn) != CONNECTION_OK)
-	{
-		*err = pchomp(PQerrorMessage(conn->streamConn));
-		return NULL;
-	}
+		goto bad_connection_errmsg;
 
 	if (logical)
 	{
@@ -234,9 +228,9 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 		if (PQresultStatus(res) != PGRES_TUPLES_OK)
 		{
 			PQclear(res);
-			ereport(ERROR,
-					(errmsg("could not clear search path: %s",
-							pchomp(PQerrorMessage(conn->streamConn)))));
+			*err = psprintf(_("could not clear search path: %s"),
+							pchomp(PQerrorMessage(conn->streamConn)));
+			goto bad_connection;
 		}
 		PQclear(res);
 	}
@@ -244,6 +238,16 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
 	conn->logical = logical;
 
 	return conn;
+
+	/* error path, using libpq's error message */
+bad_connection_errmsg:
+	*err = pchomp(PQerrorMessage(conn->streamConn));
+
+	/* error path, error already set */
+bad_connection:
+	PQfinish(conn->streamConn);
+	pfree(conn);
+	return NULL;
 }
 
 /*
-- 
2.38.0


--pn27tsqzgtum5bj3--





^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: Redundant Result node
@ 2024-08-23 02:31  Richard Guo <[email protected]>
  0 siblings, 0 replies; 10+ messages in thread

From: Richard Guo @ 2024-08-23 02:31 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers

On Thu, Aug 22, 2024 at 8:03 PM David Rowley <[email protected]> wrote:
> On Thu, 22 Aug 2024 at 23:33, Peter Eisentraut <[email protected]> wrote:
> > > I wonder if we need to invent a function to compare two PathTargets.
> >
> > Wouldn't the normal node equal() work?
>
> It might. I think has_volatile_expr might be missing a
> pg_node_attr(equal_ignore).

Yeah, maybe we can make the node equal() work for PathTarget.  We'll
need to remove the no_equal attribute in PathTarget.  I think we also
need to specify pg_node_attr(equal_ignore) for PathTarget.cost.

BTW, I'm wondering why we specify no_copy for PathTarget, while
meanwhile implementing a separate function copy_pathtarget() in
tlist.c to copy a PathTarget.  Can't we support copyObject() for
PathTarget?

Also the pg_node_attr(array_size(exprs)) attribute for
PathTarget.sortgrouprefs does not seem right to me.  In a lot of cases
sortgrouprefs would just be NULL.  Usually it is valid only for
upper-level Paths.  Hmm, maybe this is why we do not support
copyObject() for PathTarget?

Thanks
Richard






^ permalink  raw  reply  [nested|flat] 10+ messages in thread


end of thread, other threads:[~2024-08-23 02:31 UTC | newest]

Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-28 14:30 Extend compatibility of PostgreSQL::Test::Cluster Andrew Dunstan <[email protected]>
2021-12-28 16:46 ` Andrew Dunstan <[email protected]>
2021-12-31 16:20   ` Dagfinn Ilmari Mannsåker <[email protected]>
2021-12-31 16:22     ` Andrew Dunstan <[email protected]>
2022-03-29 21:56       ` Andrew Dunstan <[email protected]>
2022-03-30 05:55         ` Michael Paquier <[email protected]>
2022-03-30 15:27           ` Andrew Dunstan <[email protected]>
2023-01-21 02:27 [PATCH v3 1/4] Fix error handling in libpqrcv_connect() Andres Freund <[email protected]>
2023-01-21 02:27 [PATCH v3 1/4] Fix error handling in libpqrcv_connect() Andres Freund <[email protected]>
2024-08-23 02:31 Re: Redundant Result node Richard Guo <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox