Request for removal of BUG #5705 from todo items as no repro

[Ankit Kumar Pandey]

Hi,


This is in reference to BUG #5705 and corresponding todo item: Fix 
/contrib/btree_gist's implementation of inet indexing


Issue: SELECT '1.255.255.200/8'::inet < '1.0.0.0'::inet didn't worked 
with index.


I am not able to repro this issue.

Steps:

SELECT '1.255.255.200/8'::inet < '1.0.0.0'::inet;
 ?column?
--
 t
(1 row)

CREATE TABLE inet_test (a inet);
INSERT INTO inet_test VALUES ('1.255.255.200/8');

SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
    a
-
 1.255.255.200/8
(1 row)

EXPLAIN ANALYZE SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
 QUERY PLAN

 Seq Scan on inet_test  (cost=0.00..1.01 rows=1 width=32) (actual 
time=0.032..0.033 rows=1 loops=1)

   Filter: (a < '1.0.0.0'::inet)
 Planning Time: 0.040 ms
 Execution Time: 0.049 ms
(4 rows)

UPDATE pg_opclass SET opcdefault=true WHERE opcname = 'inet_ops';

CREATE INDEX inet_test_idx ON inet_test USING gist (a);
SET enable_seqscan = false;

SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
    a
-
 1.255.255.200/8

## This was expected to return 0 rows as in bug report

EXPLAIN analyze SELECT * FROM inet_test WHERE a < '1.0.0.0'::inet;
  QUERY PLAN

--
-
 Index Only Scan using inet_test_idx on inet_test (cost=0.12..8.14 
rows=1 width=32) (actual time=0.024..0.025 rows=1 loop

s=1)
   Index Cond: (a < '1.0.0.0'::inet)
   Heap Fetches: 1
 Planning Time: 0.056 ms
 Execution Time: 0.044 ms
(5 rows)

Gist index works fine as opposed to issue reported in the bug. Bug 
should be marked as resolved and todo item can be removed.


--
Regards,
Ankit Kumar Pandey

Re: split TOAST support out of postgres.h

[Andrew Dunstan]

On 2022-12-30 Fr 11:50, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 28.12.22 16:07, Tom Lane wrote:
>>> I dunno, #3 seems kind of unprincipled.  Also, since fmgr.h is included
>>> so widely, I doubt it is buying very much in terms of reducing header
>>> footprint.  How bad is it to do #2?
>> See this incremental patch set.
> Wow, 41 files requiring varatt.h is a lot fewer than I would have guessed.
> I think that bears out my feeling that fmgr.h wasn't a great location:
> I count 117 #includes of that, many of which are in .h files themselves
> so that many more .c files would be required to read them.
>
> (You did check that this passes cpluspluscheck/headerscheck, right?)
>
>> It seems like maybe there is some intermediate abstraction that a lot of 
>> these places should be using that we haven't thought of yet.
> Hmm.  Perhaps, but I think I'm content with this version of the patch.


Looked good to me too.


cheers


andrew

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

[Tomas Vondra]

On 12/31/22 05:42, Tom Lane wrote:
> Tomas Vondra  writes:
>> After thinking about it a bit more I decided to rip out the 10% sampling
>> rate inflation.
> 
> +1.  I'm not sure if there's anything more we need to do there, but
> that didn't seem like that was it.
> 
> I notice that the committed patch still has a reference to that hack
> though:
> 
> +* Ensure the sampling rate is between 0.0 and 1.0, even after the
> +* 10% adjustment above.  (Clamping to 0.0 is just paranoia.)
> 
> Clamping still seems like a wise idea, but the comment is just
> confusing now.
> 

Yeah, I missed that reference. Will fix.

> Also, I wonder if there is any possibility of ANALYZE failing
> with
> 
> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized 
> views
> 
> I think the patch avoids that, but only accidentally, because
> reltuples will be 0 or -1 for a view.  Maybe it'd be a good
> idea to pull back relkind along with reltuples, and check
> that too?

Not sure. I guess we can rely on reltuples being 0 or -1 in such cases,
but maybe it'd be good to at least mention that in a comment? We're not
going to use other reltuples values for views etc.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Announcing Release 15 of the PostgreSQL Buildfarm client

[Andrew Dunstan]

Changes

  * add a new script |manage_alerts.pl| that lets the user enable or
disable alerts for an animal
This is especially useful in the case of animals that have stopped
running for some reason.
  * check if a branch is up to date before trying to run it
This only applies if the |branches_to_build| setting is a keyword
rather than a list of branches. It reduces the number of useless
calls to |git pull| to almost zero.
  * require Perl version 5.14 or later
This should not be a problem, as it's more than 10 years old.
  * add |--avoid-ts-collisions| command line parameter
This is for specialized uses, and imposes a penalty of a few seconds
per run. |run_branches.pl| already does this, so it's not required for
normal operations.
  * run TAP tests for |src/interfaces| subdirectories
  * add amcheck and extension upgrade tests to cross version upgrade testing
  * adjust to changes in postgres code, file locations, etc.
  * assorted minor bug fixes and tweaks


The release can be downloaded from

 or


Upgrading is highly recommended.


cheers


andrew

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

Re: postgres_fdw: using TABLESAMPLE to collect remote sample

[Tom Lane]

Tomas Vondra  writes:
> On 12/31/22 05:42, Tom Lane wrote:
>> ERROR:  TABLESAMPLE clause can only be applied to tables and materialized 
>> views
>> I think the patch avoids that, but only accidentally, because
>> reltuples will be 0 or -1 for a view.  Maybe it'd be a good
>> idea to pull back relkind along with reltuples, and check
>> that too?

> Not sure. I guess we can rely on reltuples being 0 or -1 in such cases,
> but maybe it'd be good to at least mention that in a comment? We're not
> going to use other reltuples values for views etc.

Would anyone ever point a foreign table at a sequence?  I guess it
would be a weird use-case, but it's possible, or was till now.

regards, tom lane

Re: Request for removal of BUG #5705 from todo items as no repro

[Tom Lane]

Ankit Kumar Pandey  writes:
> This is in reference to BUG #5705 and corresponding todo item: Fix 
> /contrib/btree_gist's implementation of inet indexing

> I am not able to repro this issue.

You didn't test it right: the complaint is about the btree_gist
extension, not the in-core inet opclass, which didn't even
exist when this bug was filed.  AFAICS btree_gist is still
broken.  See

https://www.postgresql.org/message-id/flat/201010112055.o9BKtZf7011251%40wwwmaster.postgresql.org

The commit message for f23a5630e may also be informative:

https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=f23a5630e

regards, tom lane

Re: Request for removal of BUG #5705 from todo items as no repro

[Ankit Kumar Pandey]

On 31/12/22 23:32, Tom Lane wrote:

Ankit Kumar Pandey  writes:

This is in reference to BUG #5705 and corresponding todo item: Fix
/contrib/btree_gist's implementation of inet indexing
I am not able to repro this issue.

You didn't test it right: the complaint is about the btree_gist
extension, not the in-core inet opclass, which didn't even
exist when this bug was filed.  AFAICS btree_gist is still
broken.  See

https://www.postgresql.org/message-id/flat/201010112055.o9BKtZf7011251%40wwwmaster.postgresql.org

The commit message for f23a5630e may also be informative:

https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=f23a5630e

regards, tom lane


Hi,

Sorry I missed this. Thanks for the pointer, I will check this again 
properly.


--
Regards,
Ankit Kumar Pandey

[PATCH] Add overlaps geometric operators that ignore point overlaps

[Ankit Kumar Pandey]

Hi,

This is patch for todo item: Add overlaps geometric operators that 
ignore point overlaps


Issue:

SELECT circle '((0,0), 1)' && circle '((2,0),1) returns True

Expectation: In above case, both figures touch other but do not overlap 
(i.e. touching != overlap). Hence, it should return false.


Cause:

Less than or equal check between distance of center and sum of radius

Datum
circle_overlap(PG_FUNCTION_ARGS)
{
    CIRCLE       *circle1 = PG_GETARG_CIRCLE_P(0);
    CIRCLE       *circle2 = PG_GETARG_CIRCLE_P(1);

    PG_RETURN_BOOL(FPle(point_dt(&circle1->center, &circle2->center),
                    float8_pl(circle1->radius, circle2->radius)));
}

Possible fix:

# Don't check for <= , just < would suffice.

Datum
circle_overlap(PG_FUNCTION_ARGS)
{
    CIRCLE       *circle1 = PG_GETARG_CIRCLE_P(0);
    CIRCLE       *circle2 = PG_GETARG_CIRCLE_P(1);

    PG_RETURN_BOOL(FPlt(point_dt(&circle1->center, &circle2->center),
                    float8_pl(circle1->radius, circle2->radius)));
}

same for boxes as well.

Results:

Before:

select box '((0,0),(1,1))' && box '((0,1), (1,2))';
 ?column?
--
 t
(1 row)

With patch:

select box '((0,1),(1,1))' && box '((1,1), (1,2))';
 ?column?
--
 f
(1 row)

Bring box slightly ( > EPSILON) inside the other box

select box '((0,0),(1,1.0001))' && box '((0,1), (1,2))';
 ?column?
--
 t
(1 row)

similar for circle.


Now, as per as discussion 
(https://www.postgresql.org/message-id/20100322175532.GG26428%40fetter.org) 
and corresponding change in docs, 
https://www.postgresql.org/docs/15/functions-geometry.html, it mentions


`Do these objects overlap? (One point in common makes this true.) `. 
Does this means current behavior is correct? Or do we still need the 
proposed change (if so, with proper updates in docs)?


If current behavior is correct, this todo item might need some update 
(unless I missed anything) otherwise any suggestion is welcomed.


Also, I did some search around this and there is general sense of 
differentiation between overlap and touch of geometric figures. I am not 
able to find any function which can determine if two geometric figures 
touch each


other at a point (and if there is real use case of this).

In any case, patch attached for a reference. Any feedback is welcomed.


--
Regards,
Ankit Kumar Pandey
From 1c180b50493c416d063bca9ed0a1669c2b1b191d Mon Sep 17 00:00:00 2001
From: Ankit Kumar Pandey 
Date: Sun, 1 Jan 2023 00:25:06 +0530
Subject: [PATCH] Ignore point overlap operation

Geometric figure does not overlap if they just
touch each other. This patch removes requirement of point overlap for
overlap condition.
---
 doc/src/sgml/func.sgml  |  2 +-
 src/backend/utils/adt/geo_ops.c | 10 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3bf8d021c3..d1abd545a4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -11209,7 +11209,7 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
 boolean


-Do these objects overlap?  (One point in common makes this true.)
+Do these objects overlap?  (One point in common does not count as overlap.)
 Available for box, polygon,
 circle.

diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 86da025906..6a06abfd2c 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -570,10 +570,10 @@ box_overlap(PG_FUNCTION_ARGS)
 static bool
 box_ov(BOX *box1, BOX *box2)
 {
-	return (FPle(box1->low.x, box2->high.x) &&
-			FPle(box2->low.x, box1->high.x) &&
-			FPle(box1->low.y, box2->high.y) &&
-			FPle(box2->low.y, box1->high.y));
+	return (FPlt(box1->low.x, box2->high.x) &&
+			FPlt(box2->low.x, box1->high.x) &&
+			FPlt(box1->low.y, box2->high.y) &&
+			FPlt(box2->low.y, box1->high.y));
 }
 
 /*		box_left		-		is box1 strictly left of box2?
@@ -4765,7 +4765,7 @@ circle_overlap(PG_FUNCTION_ARGS)
 	CIRCLE	   *circle1 = PG_GETARG_CIRCLE_P(0);
 	CIRCLE	   *circle2 = PG_GETARG_CIRCLE_P(1);
 
-	PG_RETURN_BOOL(FPle(point_dt(&circle1->center, &circle2->center),
+	PG_RETURN_BOOL(FPlt(point_dt(&circle1->center, &circle2->center),
 		float8_pl(circle1->radius, circle2->radius)));
 }
 
-- 
2.37.2

Re: New strategies for freezing, advancing relfrozenxid early

[Jeff Davis]

On Fri, 2022-12-30 at 16:58 -0800, Peter Geoghegan wrote:
> Following the path of freezing a page is *always* valid, by
> definition. Including when there are zero freeze plans to execute, or
> even zero tuples to examine in the first place -- we'll at least be
> able to perform nominal freezing, no matter what.

This is a much clearer description, in my opinion. Do you think this is
already reflected in the comments (and I missed it)?

Perhaps the comment in the "if (tuples_frozen == 0)" branch could be
something more like:

"We have no freeze plans to execute, so there's no cost to following
the freeze path. This is important in the case where the page is
entirely frozen already, so that the page will be marked as such in the
VM."

I'm not even sure we really want a new concept of "nominal freezing". I
think you are right to just call it a degenerate case where it can be
interpreted as either freezing zero things or not freezing; and the
former is convenient for us because we want to follow that code path.
That would be another good way of writing the comment, in my opinion.

Of course, I'm sure there are some nuances that I'm still missing.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

Re: New strategies for freezing, advancing relfrozenxid early

[Peter Geoghegan]

On Sat, Dec 31, 2022 at 11:46 AM Jeff Davis  wrote:
> On Fri, 2022-12-30 at 16:58 -0800, Peter Geoghegan wrote:
> > Following the path of freezing a page is *always* valid, by
> > definition. Including when there are zero freeze plans to execute, or
> > even zero tuples to examine in the first place -- we'll at least be
> > able to perform nominal freezing, no matter what.
>
> This is a much clearer description, in my opinion. Do you think this is
> already reflected in the comments (and I missed it)?

I am arguably the person least qualified to answer this question.   :-)

> Perhaps the comment in the "if (tuples_frozen == 0)" branch could be
> something more like:
>
> "We have no freeze plans to execute, so there's no cost to following
> the freeze path. This is important in the case where the page is
> entirely frozen already, so that the page will be marked as such in the
> VM."

I'm happy to use your wording instead -- I'll come up with a patch for that.

In my mind it's just a restatement of what's there already. I assume
that you're right about it being clearer this way.

> Of course, I'm sure there are some nuances that I'm still missing.

I don't think that there is, actually. I now believe that you totally
understand the mechanics involved here. I'm glad that I was able to
ascertain that that's all it was. It's worth going to the trouble of
getting something like this exactly right.

-- 
Peter Geoghegan

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Fri, Dec 30, 2022 at 2:17 PM Corey Huinker 
wrote:

> On Wed, Dec 28, 2022 at 5:59 AM Maxim Orlov  wrote:
>
>> Hi!
>>
>> The patch is implementing what is declared to do. Shell return code is
>> now accessible is psql var.
>> Overall code is in a good condition. Applies with no errors on master.
>> Unfortunately, regression tests are failing on the macOS due to the
>> different shell output.
>>
>
> That was to be expected.
>
> I wonder if there is value in setting up a psql on/off var
> SHELL_ERROR_OUTPUT construct that when set to "off/false"
> suppresses standard error via appending "2> /dev/null" (or "2> nul" if
> #ifdef WIN32). At the very least, it would allow for tests like this to be
> done with standard regression scripts.
>

Thinking on this some more a few ideas came up:

1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations,
but it would fail if the user took it upon themselves to redirect output,
and suddenly "myprog 2> /dev/null" works fine on its own but will fail when
we append our own "2> /dev/null" to it.

2. Exposing a PSQL var that identifies the shell snippet for "how to
suppress standard error", which would be "2> /dev/null" for -nix systems
and "2> NUL" for windows systems. This again, presumes that users will
actually need this feature, and I'm not convinced that they will.

3. Exposing a PSQL var that is basically an "is this environment running on
windows" and let them construct it from there. That gets complicated with
WSL and the like, so I'm not wild about this, even though it would be
comparatively simple to implement.

4. We could inject an environment variable into our own regression tests
directly based on the "#ifdef WIN32" in our own code, and we can use a
couple of \gset commands to construct the error-suppressed shell commands
as needed. This seems like the best starting point, and we can always turn
that env var into a real variable if it proves useful elsewhere.

Re: Add SHELL_EXIT_CODE to psql

[Isaac Morland]

On Sat, 31 Dec 2022 at 16:47, Corey Huinker  wrote:

>
>> I wonder if there is value in setting up a psql on/off var
>> SHELL_ERROR_OUTPUT construct that when set to "off/false"
>> suppresses standard error via appending "2> /dev/null" (or "2> nul" if
>> #ifdef WIN32). At the very least, it would allow for tests like this to be
>> done with standard regression scripts.
>>
>
> Thinking on this some more a few ideas came up:
>
> 1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations,
> but it would fail if the user took it upon themselves to redirect output,
> and suddenly "myprog 2> /dev/null" works fine on its own but will fail when
> we append our own "2> /dev/null" to it.
>

Rather than attempting to append redirection directives to the command,
would it work to redirect stderr before invoking the shell? This seems to
me to be more reliable and it should allow an explicit redirection in the
shell command to still work. The difference between Windows and Unix then
becomes the details of what system calls we use to accomplish the
redirection (or maybe none, if an existing abstraction layer takes care of
that - I'm not really up on Postgres internals enough to know), rather than
what we append to the provided command.

Re: wake up logical workers after ALTER SUBSCRIPTION

[Nathan Bossart]

On Sun, Dec 18, 2022 at 03:36:07PM -0800, Nathan Bossart wrote:
> This seems to have somehow broken the archiving tests on Windows, so
> obviously I owe some better analysis here.  I didn't see anything obvious
> in the logs, but I will continue to dig.

On Windows, WaitForWALToBecomeAvailable() seems to depend on the call to
WaitLatch() for wal_retrieve_retry_interval to ensure that signals are
dispatched (i.e., pgwin32_dispatch_queued_signals()).  My first instinct is
to just always call WaitLatch() in this code path, even if
wal_retrieve_rety_interval milliseconds have already elapsed.  The attached
0003 does this.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From d1025a5d7ad9a966f7ec8bee4bb8127e8a0e1d8b Mon Sep 17 00:00:00 2001
From: Nathan Bossart 
Date: Mon, 21 Nov 2022 16:01:01 -0800
Subject: [PATCH v10 1/4] wake up logical workers as needed instead of relying
 on periodic wakeups

---
 src/backend/access/transam/xact.c   |  3 ++
 src/backend/commands/alter.c|  7 
 src/backend/commands/subscriptioncmds.c |  4 ++
 src/backend/replication/logical/tablesync.c | 10 +
 src/backend/replication/logical/worker.c| 46 +
 src/include/replication/logicalworker.h |  3 ++
 6 files changed, 73 insertions(+)

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b7c7fd9f00..70ad51c591 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -47,6 +47,7 @@
 #include "pgstat.h"
 #include "replication/logical.h"
 #include "replication/logicallauncher.h"
+#include "replication/logicalworker.h"
 #include "replication/origin.h"
 #include "replication/snapbuild.h"
 #include "replication/syncrep.h"
@@ -2360,6 +2361,7 @@ CommitTransaction(void)
 	AtEOXact_PgStat(true, is_parallel_worker);
 	AtEOXact_Snapshot(true, false);
 	AtEOXact_ApplyLauncher(true);
+	AtEOXact_LogicalRepWorkers(true);
 	pgstat_report_xact_timestamp(0);
 
 	CurrentResourceOwner = NULL;
@@ -2860,6 +2862,7 @@ AbortTransaction(void)
 		AtEOXact_HashTables(false);
 		AtEOXact_PgStat(false, is_parallel_worker);
 		AtEOXact_ApplyLauncher(false);
+		AtEOXact_LogicalRepWorkers(false);
 		pgstat_report_xact_timestamp(0);
 	}
 
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 10b6fe19a2..d095cd3ced 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -59,6 +59,7 @@
 #include "commands/user.h"
 #include "miscadmin.h"
 #include "parser/parse_func.h"
+#include "replication/logicalworker.h"
 #include "rewrite/rewriteDefine.h"
 #include "tcop/utility.h"
 #include "utils/builtins.h"
@@ -279,6 +280,12 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
 		if (strncmp(new_name, "regress_", 8) != 0)
 			elog(WARNING, "subscriptions created by regression test cases should have names starting with \"regress_\"");
 #endif
+
+		/*
+		 * Wake up the logical replication workers to handle this change
+		 * quickly.
+		 */
+		LogicalRepWorkersWakeupAtCommit(objectId);
 	}
 	else if (nameCacheId >= 0)
 	{
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d673557ea4..d6993c26e5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -34,6 +34,7 @@
 #include "nodes/makefuncs.h"
 #include "pgstat.h"
 #include "replication/logicallauncher.h"
+#include "replication/logicalworker.h"
 #include "replication/origin.h"
 #include "replication/slot.h"
 #include "replication/walreceiver.h"
@@ -1362,6 +1363,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 	InvokeObjectPostAlterHook(SubscriptionRelationId, subid, 0);
 
+	/* Wake up the logical replication workers to handle this change quickly. */
+	LogicalRepWorkersWakeupAtCommit(subid);
+
 	return myself;
 }
 
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 94e813ac53..509fe2eb19 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -105,6 +105,7 @@
 #include "pgstat.h"
 #include "replication/logicallauncher.h"
 #include "replication/logicalrelation.h"
+#include "replication/logicalworker.h"
 #include "replication/walreceiver.h"
 #include "replication/worker_internal.h"
 #include "replication/slot.h"
@@ -619,6 +620,15 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
 
 	if (started_tx)
 	{
+		/*
+		 * If we are ready to enable two_phase mode, wake up the logical
+		 * replication workers to handle this change quickly.
+		 */
+		CommandCounterIncrement();
+		if (MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING &&
+			AllTablesyncsReady())
+			LogicalRepWorkersWakeupAtCommit(MyLogicalRepWorker->subid);
+
 		CommitTransactionCommand();
 		pgstat_report_stat(true);
 	}
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logic

Re: Add SHELL_EXIT_CODE to psql

[Corey Huinker]

On Sat, Dec 31, 2022 at 5:28 PM Isaac Morland 
wrote:

> On Sat, 31 Dec 2022 at 16:47, Corey Huinker 
> wrote:
>
>>
>>> I wonder if there is value in setting up a psql on/off var
>>> SHELL_ERROR_OUTPUT construct that when set to "off/false"
>>> suppresses standard error via appending "2> /dev/null" (or "2> nul" if
>>> #ifdef WIN32). At the very least, it would allow for tests like this to be
>>> done with standard regression scripts.
>>>
>>
>> Thinking on this some more a few ideas came up:
>>
>> 1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations,
>> but it would fail if the user took it upon themselves to redirect output,
>> and suddenly "myprog 2> /dev/null" works fine on its own but will fail when
>> we append our own "2> /dev/null" to it.
>>
>
> Rather than attempting to append redirection directives to the command,
> would it work to redirect stderr before invoking the shell? This seems to
> me to be more reliable and it should allow an explicit redirection in the
> shell command to still work. The difference between Windows and Unix then
> becomes the details of what system calls we use to accomplish the
> redirection (or maybe none, if an existing abstraction layer takes care of
> that - I'm not really up on Postgres internals enough to know), rather than
> what we append to the provided command.
>
>
Inside psql, it's a call to the system() function which takes a single
string. All output, stdout or stderr, is printed. So for the regression
test we have to compose a command that is OS appropriate AND suppresses
stderr.

Fixing a couple of buglets in how VACUUM sets visibility map bits

[Peter Geoghegan]

Attached are two patches, each of which fixes two historical buglets
around VACUUM's approach to setting bits in the visibility map.
(Whether or not this is actually refactoring work or hardening work is
debatable, I suppose.)

The first patch makes sure that the snapshotConflictHorizon cutoff
(XID cutoff for recovery conflicts) is never a special XID, unless
that XID is InvalidTransactionId, which is interpreted as a
snapshotConflictHorizon value that will never need a recovery conflict
(per the general convention for snapshotConflictHorizon values
explained above ResolveRecoveryConflictWithSnapshot). This patch
establishes a hard rule that snapshotConflictHorizon values can never
be a special XID value, unless it's InvalidTransactionId. An assertion
enforces the rule for us in REDO routines (at the point that they call
ResolveRecoveryConflictWithSnapshot with the WAL record's
snapshotConflictHorizon XID value).

The second patch makes sure that VACUUM can never set a page
all-frozen in the visibility map without also setting the same page
all-visible in the same call to visibilitymap_set() -- regardless of
what we think we know about the current state of the all-visible bit
in the VM.

The second patch adjusts one of the visibilitymap_set() calls in
vacuumlazy.c that would previously sometimes set a page's all-frozen
bit without also setting its all-visible bit. This could allow VACUUM
to leave a page all-frozen but not all-visible in the visibility map
(since the value of all_visible_according_to_vm can go stale). I think
that this should be treated as a basic visibility map invariant: an
all-frozen page must also be all-visible, by definition, so why should
it be physically possible for the VM to give a contradictory picture
of the all_visible/all_frozen status of any one page? Assertions are
added that more or less make this rule into an invariant.
amcheck/pg_visibility coverage might make sense too, but I haven't
done that here.

The second patch also adjusts a later visibilitymap_set() call site
(the one used just after heap vacuuming runs in the final heap pass)
in roughly the same way. It no longer reads from the visibility map to
see what bits need to be changed. The existing approach here seems
rather odd. The whole point of calling lazy_vacuum_heap_page() is to
set LP_DEAD items referenced by VACUUM's dead_items array to LP_UNUSED
-- there has to have been at least one LP_DEAD item on the page for us
to end up here (which a Postgres 14 era assertion verifies for us). So
we already know perfectly well that the visibility map shouldn't
indicate that the page is all-visible yet -- why bother asking the VM?
And besides, any call to visibilitymap_set() will only modify the VM
when it directly observes that the bits have changed -- so why even
attempt to duplicate that on the caller side?

It seems to me that the visibilitymap_get_status() call inside
lazy_vacuum_heap_page() is actually abused to work as a substitute for
visibilitymap_pin(). Why not use top-level visibilitymap_pin() calls
instead, just like we do it in the first heap pass? That's how it's
done in the second patch; it adds a visibilitymap_pin() call in
lazy_vacuum_heap_rel()'s blkno-wise loop. That gives us parity between
the first and second heap pass, which seems like a clear
maintainability win -- everybody can pass the
already-pinned/already-setup vmbuffer by value.

-- 
Peter Geoghegan


v1-0001-Avoid-special-XIDs-in-snapshotConflictHorizon-val.patch
Description: Binary data


v1-0002-Never-just-set-the-all-frozen-bit-in-VM.patch
Description: Binary data

Re: Announcing Release 15 of the PostgreSQL Buildfarm client

[Noah Misch]

On Sat, Dec 31, 2022 at 10:02:32AM -0500, Andrew Dunstan wrote:
>   * check if a branch is up to date before trying to run it
> This only applies if the |branches_to_build| setting is a keyword
> rather than a list of branches. It reduces the number of useless
> calls to |git pull| to almost zero.

This new reliance on buildfarm.postgresql.org/branches_of_interest.json is
trouble for non-SSL buildfarm animals.
http://buildfarm.postgresql.org/branches_of_interest.txt has an exemption to
allow serving over plain http, but the json URL just redirects the client to
https.  Can the json file get the same exemption-from-redirect that the txt
file has?

Re: Infinite Interval

[Vik Fearing]

On 12/31/22 06:09, jian he wrote:


Since in float8 you can use '+inf', '+infinity', So should we also make
interval '+infinity' valid?


Yes.


Also in timestamp. '+infinity'::timestamp is invalid, should we make it
valid.


Yes, we should.  I wrote a trivial patch for this a while ago but it 
appears I never posted it.  I will post that in a new thread so as not 
to confuse the bots.

--
Vik Fearing

+infinity for dates and timestamps

[Vik Fearing]

It has always annoyed me that we can't write '+infinity' for dates and 
timestamps and get the OCD satisfaction of making our queries line up 
with '-infinity'.


I wrote a fix for that some time ago but apparently never posted it.  I 
was reminded of it by jian he in the Infinite Interval thread, and so 
here it is.

--
Vik Fearingcommit 5178a17a3280bc0018194e590d1b9fb3afbe3b65
Author: Vik Fearing 
Date:   Tue Jun 7 00:22:21 2022 +0200

allow +infinity for dates

diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index fdffba4442..863ab84442 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2288,7 +2288,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   PostgreSQL supports several
   special date/time input values for convenience, as shown in .  The values
-  infinity and -infinity
+  infinity (alternatively +infinity) and -infinity
   are specially represented inside the system and will be displayed
   unchanged; but the others are simply notational shorthands
   that will be converted to ordinary date/time values when read.
@@ -2315,7 +2315,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   1970-01-01 00:00:00+00 (Unix system time zero)
  
  
-  infinity
+  infinity or +infinity
   date, timestamp
   later than all other time stamps
  
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 8afda0e5d2..6b2da1acfe 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -105,6 +105,7 @@ const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
  */
 static const datetkn datetktbl[] = {
 	/* token, type, value */
+	{LATE2, RESERV, DTK_LATE},	/* "+infinity" reserved for "late time" */
 	{EARLY, RESERV, DTK_EARLY}, /* "-infinity" reserved for "early time" */
 	{DA_D, ADBC, AD},			/* "ad" for years > 0 */
 	{"allballs", RESERV, DTK_ZULU}, /* 00:00:00 */
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index bdb7c06bec..d503d8b24d 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -38,6 +38,7 @@ struct tzEntry;
 #define INVALID			"invalid"
 #define EARLY			"-infinity"
 #define LATE			"infinity"
+#define LATE2			"+infinity"
 #define NOW"now"
 #define TODAY			"today"
 #define TOMORROW		"tomorrow"
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 9fd15be5f9..1dcb07b78f 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -329,6 +329,7 @@ select 'infinity'::date, '-infinity'::date;
 select 'infinity'::date > 'today'::date as t;
 select '-infinity'::date < 'today'::date as t;
 select isfinite('infinity'::date), isfinite('-infinity'::date), isfinite('today'::date);
+select 'infinity'::date = '+infinity'::date as t;
 --
 -- oscillating fields from non-finite date:
 --
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index e1175b12ce..004d1d3454 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -49,6 +49,10 @@ INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
 
+-- Make sure 'infinity' and '+infinity' are the same
+SELECT timestamp 'infinity' = timestamp '+infinity' AS t;
+SELECT timestamptz 'infinity' = timestamptz '+infinity' AS t;
+
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 

Re: Announcing Release 15 of the PostgreSQL Buildfarm client

[Andrew Dunstan]

On 2022-12-31 Sa 20:55, Noah Misch wrote:
> On Sat, Dec 31, 2022 at 10:02:32AM -0500, Andrew Dunstan wrote:
>>   * check if a branch is up to date before trying to run it
>> This only applies if the |branches_to_build| setting is a keyword
>> rather than a list of branches. It reduces the number of useless
>> calls to |git pull| to almost zero.
> This new reliance on buildfarm.postgresql.org/branches_of_interest.json is
> trouble for non-SSL buildfarm animals.
> http://buildfarm.postgresql.org/branches_of_interest.txt has an exemption to
> allow serving over plain http, but the json URL just redirects the client to
> https.  Can the json file get the same exemption-from-redirect that the txt
> file has?


I didn't realize there were animals left other than mine which had this
issue. I asked the admins some weeks ago to fix this (I don't have
privilege to do so), but have not had a response yet. The temporary
workaround is to use a list of named branches, e.g. instead of 'ALL' use
[qw(REL_11_STABLE REL_12_STABLE REL_13_STABLE REL_14_STABLE
REL_15_STABLE HEAD)]


cheers


andrew


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

Re: +infinity for dates and timestamps

[Vik Fearing]

On 1/1/23 03:10, Vik Fearing wrote:
It has always annoyed me that we can't write '+infinity' for dates and 
timestamps and get the OCD satisfaction of making our queries line up 
with '-infinity'.


I wrote a fix for that some time ago but apparently never posted it.  I 
was reminded of it by jian he in the Infinite Interval thread, and so 
here it is.


Hmm.  Somehow the .out test files were not included.

Fixed with attached.
--
Vik Fearing
commit 5178a17a3280bc0018194e590d1b9fb3afbe3b65
Author: Vik Fearing 
Date:   Tue Jun 7 00:22:21 2022 +0200

allow +infinity for dates

diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index fdffba4442..863ab84442 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2288,7 +2288,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   PostgreSQL supports several
   special date/time input values for convenience, as shown in .  The values
-  infinity and -infinity
+  infinity (alternatively +infinity) and -infinity
   are specially represented inside the system and will be displayed
   unchanged; but the others are simply notational shorthands
   that will be converted to ordinary date/time values when read.
@@ -2315,7 +2315,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   1970-01-01 00:00:00+00 (Unix system time zero)
  
  
-  infinity
+  infinity or +infinity
   date, timestamp
   later than all other time stamps
  
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 8afda0e5d2..6b2da1acfe 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -105,6 +105,7 @@ const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
  */
 static const datetkn datetktbl[] = {
 	/* token, type, value */
+	{LATE2, RESERV, DTK_LATE},	/* "+infinity" reserved for "late time" */
 	{EARLY, RESERV, DTK_EARLY}, /* "-infinity" reserved for "early time" */
 	{DA_D, ADBC, AD},			/* "ad" for years > 0 */
 	{"allballs", RESERV, DTK_ZULU}, /* 00:00:00 */
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index bdb7c06bec..d503d8b24d 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -38,6 +38,7 @@ struct tzEntry;
 #define INVALID			"invalid"
 #define EARLY			"-infinity"
 #define LATE			"infinity"
+#define LATE2			"+infinity"
 #define NOW"now"
 #define TODAY			"today"
 #define TOMORROW		"tomorrow"
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 9fd15be5f9..1dcb07b78f 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -329,6 +329,7 @@ select 'infinity'::date, '-infinity'::date;
 select 'infinity'::date > 'today'::date as t;
 select '-infinity'::date < 'today'::date as t;
 select isfinite('infinity'::date), isfinite('-infinity'::date), isfinite('today'::date);
+select 'infinity'::date = '+infinity'::date as t;
 --
 -- oscillating fields from non-finite date:
 --
diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql
index e1175b12ce..004d1d3454 100644
--- a/src/test/regress/sql/timestamp.sql
+++ b/src/test/regress/sql/timestamp.sql
@@ -49,6 +49,10 @@ INSERT INTO TIMESTAMP_TBL VALUES ('-infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('infinity');
 INSERT INTO TIMESTAMP_TBL VALUES ('epoch');
 
+-- Make sure 'infinity' and '+infinity' are the same
+SELECT timestamp 'infinity' = timestamp '+infinity' AS t;
+SELECT timestamptz 'infinity' = timestamptz '+infinity' AS t;
+
 -- Postgres v6.0 standard output format
 INSERT INTO TIMESTAMP_TBL VALUES ('Mon Feb 10 17:32:01 1997 PST');
 
commit 3ae288cc9684a122b08e0e61174bb63b28309110
Author: Vik Fearing 
Date:   Tue Jun 7 00:22:21 2022 +0200

allow +infinity for dates

diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml
index fdffba4442..863ab84442 100644
--- a/doc/src/sgml/datatype.sgml
+++ b/doc/src/sgml/datatype.sgml
@@ -2288,7 +2288,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   PostgreSQL supports several
   special date/time input values for convenience, as shown in .  The values
-  infinity and -infinity
+  infinity (alternatively +infinity) and -infinity
   are specially represented inside the system and will be displayed
   unchanged; but the others are simply notational shorthands
   that will be converted to ordinary date/time values when read.
@@ -2315,7 +2315,7 @@ TIMESTAMP WITH TIME ZONE '2004-10-19 10:23:54+02'
   1970-01-01 00:00:00+00 (Unix system time zero)
  
  
-  infinity
+  infinity or +infinity
   date, timestamp
   later than all other time stamps
  
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 8afda0e5d2..6b2da1acfe 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ 

Re: +infinity for dates and timestamps

[Tom Lane]

Vik Fearing  writes:
> It has always annoyed me that we can't write '+infinity' for dates and 
> timestamps and get the OCD satisfaction of making our queries line up 
> with '-infinity'.

+1, since it works for numerics it should work for these types too.
(I didn't read the patch though.)

regards, tom lane