Re: [PATCH v1] pg_ls_tmpdir to show directories
On Sat, Dec 28, 2019 at 07:52:55AM +0100, Fabien COELHO wrote:
> >>Why not simply showing the files underneath their directories?
> >>
> >> /path/to/tmp/file1
> >> /path/to/tmp/subdir1/file2
> >>
> >>In which case probably showing the directory itself is not useful,
> >>and the is_dir column could be dropped?
> >
> >The names are expected to look like this:
> >
> >$ sudo find /var/lib/pgsql/12/data/base/pgsql_tmp -ls
> >1429774 drwxr-x--- 3 postgres postgres 4096 Dec 27 13:51
> >/var/lib/pgsql/12/data/base/pgsql_tmp
> >1698684 drwxr-x--- 2 postgres postgres 4096 Dec 7 01:35
> >/var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset
> >169347 5492 -rw-r- 1 postgres postgres 5619712 Dec 7 01:35
> >/var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset/0.0
> >169346 5380 -rw-r- 1 postgres postgres 5505024 Dec 7 01:35
> >/var/lib/pgsql/12/data/base/pgsql_tmp/pgsql_tmp11025.0.sharedfileset/1.0
> >
> >I think we'd have to show sudbdir/file1, subdir/file2, not just file1, file2.
> >It doesn't seem useful or nice to show a bunch of files called 0.0 or 1.0.
> >Actually the results should be unique, either on filename or (dir,file).
>
> Ok, so this suggests recursing into subdirs, which requires to make a
> separate function of the inner loop.
Yea, it suggests that; but, SRF_RETURN_NEXT doesn't make that very easy.
It'd need to accept the fcinfo argument, and pg_ls_dir_files would call it once
for every tuple to be returned. So it's recursive and saves its state...
The attached is pretty ugly, but I can't see how to do better.
The alternative seems to be to build up a full list of pathnames in the SRF
initial branch, and stat them all later. Or stat them all in the INITIAL case,
and keep a list of stat structures to be emited during future calls.
BTW, it seems to me this error message should be changed:
snprintf(path, sizeof(path), "%s/%s", fctx->location,
de->d_name);
if (stat(path, &attrib) < 0)
ereport(ERROR,
(errcode_for_file_access(),
-errmsg("could not stat directory
\"%s\": %m", dir)));
+errmsg("could not stat file \"%s\":
%m", path)));
>From fd88be5f1687354d9990fb1838adc0db36bc6dde Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Dec 2019 23:34:14 -0600
Subject: [PATCH v2 1/2] BUG: in errmsg
---
src/backend/utils/adt/genfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 5d4f26a..c978e15 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -590,7 +590,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok)
if (stat(path, &attrib) < 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not stat directory \"%s\": %m", dir)));
+ errmsg("could not stat file \"%s\": %m", path)));
/* Ignore anything but regular files */
if (!S_ISREG(attrib.st_mode))
--
2.7.4
>From fff91aec87f635755527e91aebb7554fa6385fec Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 14 Dec 2019 16:22:15 -0600
Subject: [PATCH v2 2/2] pg_ls_tmpdir to show directories
See also 9cd92d1a33699f86aa53d44ab04cc3eb50c18d11
---
doc/src/sgml/func.sgml | 14 +++--
src/backend/utils/adt/genfile.c | 132 ++-
src/include/catalog/catversion.h | 2 +-
3 files changed, 96 insertions(+), 52 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5a98158..8abc643 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21922,12 +21922,14 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
setof record
-List the name, size, and last modification time of files in the
-temporary directory for tablespace. If
-tablespace is not provided, the
-pg_default tablespace is used. Access is granted
-to members of the pg_monitor role and may be
-granted to other non-superuser roles.
+For files in the temporary directory for
+tablespace, list the name, size, and last modification time.
+Files beneath a first-level directory are shown, and include a pathname
+component of their parent directory; such files are used by parallel processes.
+If tablespace is not provided, the
+pg_default tablespace is used. Access is granted to
+members of the pg_monitor role and may be granted to
+other non-superuser roles.
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index c978e15..2b540e9 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -37,8 +37,12 @@
typedef struct
{
- char *location;
- D
Re: Implementing Incremental View Maintenance
On Sat, Dec 28, 2019 at 12:42 AM legrand legrand wrote: > > Hello, > Thank you for this patch. > > I have tried to use an other patch with yours: > "Planning counters in pg_stat_statements (using pgss_store)" > https://www.postgresql.org/message-id/CAOBaU_Y12bn0tOdN9RMBZn29bfYYH11b2CwKO1RO7dX9fQ3aZA%40mail.gmail.com > > setting > shared_preload_libraries='pg_stat_statements' > pg_stat_statements.track=all > and creating the extension > > > When trying following syntax: > > create table b1 (id integer, x numeric(10,3)); > create incremental materialized view mv1 as select id, count(*),sum(x) from > b1 group by id; > insert into b1 values (1,1) > > I got an ASSERT FAILURE in pg_stat_statements.c > on > Assert(query != NULL); > > comming from matview.c > refresh_matview_datafill(dest_old, query, queryEnv, NULL); > or > refresh_matview_datafill(dest_new, query, queryEnv, NULL); > > > If this (last) NULL field was replaced by the query text, a comment or just > "n/a", > it would fix the problem. > > Could this be investigated ? I digged deeper into this. I found a bug in the pg_stat_statements patch, as the new pgss_planner_hook() doesn't check for a non-zero queryId, which I think should avoid that problem. This however indeed raises the question on whether the query text should be provided, and if the behavior is otherwise correct. If I understand correctly, for now this specific query won't go through parse_analysis, thus won't get a queryId and will be ignored in pgss_ExecutorEnd, so it'll be entirely invisible, except with auto_explain which will only show an orphan plan like this: 2019-12-28 12:03:29.334 CET [9399] LOG: duration: 0.180 ms plan: HashAggregate (cost=0.04..0.06 rows=1 width=60) Group Key: new_16385_0.id -> Named Tuplestore Scan (cost=0.00..0.02 rows=1 width=52)
Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders
On Thursday, December 26, 2019 8:18:46 PM CET Julien Rouhaud wrote:
> Hello Pierre,
>
> On Thu, Dec 26, 2019 at 5:43 PM Pierre Ducroquet
wrote:
> > The second one was tested on PG 10 and PG 12 (with 48 lines offset). It
> > has on PG12 the same effect it has on a PG10+isAlive patch. Instead of
> > calling each time GetFlushRecPtr, we call it only if we notice we have
> > reached the value of the previous call. This way, when the senders are
> > busy decoding, we are no longer fighting for a spinlock to read the
> > FlushRecPtr.
>
> The patch is quite straightforward and looks good to me.
>
> -XLogRecPtrflushPtr;
> +static XLogRecPtr flushPtr = 0;
>
> You should use InvalidXLogRecPtr instead though, and maybe adding some
> comments to explain why the static variable is a life changer here.
>
> > Here are some benchmark results.
> > On PG 10, to decode our replication stream, we went from 3m 43s to over 5
> > minutes after removing the first hot spot, and then down to 22 seconds.
> > On PG 12, we had to change the benchmark (due to GIN indexes creation
> > being
> > more optimized) so we can not compare directly with our previous bench. We
> > went from 15m 11s down to 59 seconds.
> > If needed, we can provide scripts to reproduce this situation. It is quite
> > simple: add ~20 walsenders doing logical replication in database A, and
> > then generate a lot of data in database B. The walsenders will be woken
> > up by the activity on database B, but not sending it thus keeping hitting
> > the same locks.
>
> Quite impressive speedup!
Hi
Thank you for your comments.
Attached to this email is a patch with better comments regarding the
XLogSendLogical change.
We've spent quite some time yesterday benching it again, this time with
changes that must be fully processed by the decoder. The speed-up is obviously
much smaller, we are only ~5% faster than without the patch.
Regardsdiff --git a/src/backend/replication/walsender.c
b/src/backend/replication/walsender.c
index 74330f8c84..fbb30c6847 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2765,7 +2765,13 @@ XLogSendLogical(void)
{
XLogRecord *record;
char *errm;
- XLogRecPtr flushPtr;
+
+ /*
+* We'll use the current flush point to determine whether we've caught
up.
+* This variable is static in order to cache it accross calls. This
caching
+* is needed because calling GetFlushRecPtr needs to acquire an
expensive lock.
+*/
+ static XLogRecPtr flushPtr = InvalidXLogRecPtr;
/*
* Don't know whether we've caught up yet. We'll set WalSndCaughtUp to
@@ -2782,11 +2788,6 @@ XLogSendLogical(void)
if (errm != NULL)
elog(ERROR, "%s", errm);
- /*
-* We'll use the current flush point to determine whether we've caught
up.
-*/
- flushPtr = GetFlushRecPtr();
-
if (record != NULL)
{
/*
@@ -2799,7 +2800,16 @@ XLogSendLogical(void)
sentPtr = logical_decoding_ctx->reader->EndRecPtr;
}
- /* Set flag if we're caught up. */
+
+ /* Initialize flushPtr if needed */
+ if (flushPtr == InvalidXLogRecPtr)
+ flushPtr = GetFlushRecPtr();
+
+ /* If EndRecPtr is past our flushPtr, we must update it to know if we
caught up */
+ if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
+ flushPtr = GetFlushRecPtr();
+
+ /* If EndRecPtr is still past our flushPtr, it means we caught up */
if (logical_decoding_ctx->reader->EndRecPtr >= flushPtr)
WalSndCaughtUp = true;
Re: use CLZ instruction in AllocSetFreeIndex()
v2 had an Assert that was only correct while experimenting with eliding right shift. Fixed in v3. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services v3-0001-Use-the-CLZ-instruction-in-AllocSetFreeIndex.patch Description: Binary data
Re: [PATCH v1] pg_ls_tmpdir to show directories
Hello Justin,
Ok, so this suggests recursing into subdirs, which requires to make a
separate function of the inner loop.
Yea, it suggests that; but, SRF_RETURN_NEXT doesn't make that very easy.
It'd need to accept the fcinfo argument, and pg_ls_dir_files would call it once
for every tuple to be returned. So it's recursive and saves its state...
The attached is pretty ugly, but I can't see how to do better.
Hmmm… I do agree with pretty ugly:-)
If it really neads to be in the structure, why not save the open directory
field in the caller and restore it after the recursive call, and pass the
rest of the structure as a pointer? Something like:
... root_function(...)
{
struct status_t status = initialization();
...
call rec_function(&status, path)
...
cleanup();
}
... rec_function(struct *status, path)
{
status->dir = opendir(path);
for (dir contents)
{
if (it_is_a_dir)
{
/* some comment about why we do that… */
dir_t saved = status->dir;
status->dir = NULL;
rec_function(status, subpath);
status->dir = saved;
}
}
closedir(status->dir), status->dir = NULL;
}
The alternative seems to be to build up a full list of pathnames in the SRF
initial branch, and stat them all later. Or stat them all in the INITIAL case,
and keep a list of stat structures to be emited during future calls.
--
Fabien.
Re: Allow WHEN in INSTEAD OF triggers
On Fri, Dec 27, 2019 at 10:29:15PM -0500, Tom Lane wrote: > David Fetter writes: > > While noodling around with an upcoming patch to remove user-modifiable > > RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF > > triggers for no discernible reason. This patch removes that > > restriction. > > This seems like a remarkably bad idea. The point of an INSTEAD OF > trigger is that it is guaranteed to handle the operation. What's > the system supposed to do with rows the trigger doesn't handle? Nothing. Why would it be different from the other forms of WHEN in triggers? > I notice that your patch doesn't even bother to test what happens, > but I'd argue that whatever it is, it's wrong. If you think that > "do nothing" or "throw an error" is appropriate, you can code that > inside the trigger. It's not PG's charter to make such a decision. If that's the case, why do we have WHEN for triggers at all? I'm just working toward make them more consistent. From a UX perspective, it's a lot simpler and clearer to do this in the trigger declaration than it is in the body. > PS: I think your chances of removing rules are not good, either. I suspect I have a lot of company in my view of user-modifiable rewrite rules as an experiment we can finally discontinue in view of its decisive results. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: Allow WHEN in INSTEAD OF triggers
On Sat, Dec 28, 2019 at 12:12:30AM -0300, Alvaro Herrera wrote:
> On 2019-Dec-28, David Fetter wrote:
>
> > While noodling around with an upcoming patch to remove user-modifiable
> > RULEs, I noticed that WHEN conditions were disallowed from INSTEAD OF
> > triggers for no discernible reason. This patch removes that
> > restriction.
>
> If you want to remove the restriction, your patch should add some test
> cases that show it working.
Tests added :)
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From d6af8b66347b31e14d961e83023dbcb658bea64b Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Fri, 27 Dec 2019 18:57:31 -0800
Subject: [PATCH v2] Allow WHEN in INSTEAD OF triggers
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.24.1"
This is a multi-part message in MIME format.
--2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
This was disallowed for reasons that aren't entirely obvious, so allow.
diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml
index 3339a4b4e1..b822cb6e8d 100644
--- a/doc/src/sgml/ref/create_trigger.sgml
+++ b/doc/src/sgml/ref/create_trigger.sgml
@@ -377,10 +377,6 @@ UPDATE OF column_name1 [, column_name2DELETE triggers cannot refer to NEW.
- INSTEAD OF triggers do not support WHEN
- conditions.
-
-
Currently, WHEN expressions cannot contain
subqueries.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 36093a29a8..c4cc07a426 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -381,17 +381,13 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("TRUNCATE FOR EACH ROW triggers are not supported")));
- /* INSTEAD triggers must be row-level, and can't have WHEN or columns */
+ /* INSTEAD triggers must be row-level, and can't have columns */
if (TRIGGER_FOR_INSTEAD(tgtype))
{
if (!TRIGGER_FOR_ROW(tgtype))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("INSTEAD OF triggers must be FOR EACH ROW")));
- if (stmt->whenClause)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("INSTEAD OF triggers cannot have WHEN conditions")));
if (stmt->columns != NIL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 1e4053ceed..f44f28760e 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -965,7 +965,11 @@ begin
end if;
if TG_OP = 'UPDATE' then
-raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+if strpos(argstr, 'instead_of_when') > 0 then
+raise NOTICE 'instead_of_when fired';
+else
+raise NOTICE 'OLD: %, NEW: %', OLD, NEW;
+end if;
UPDATE main_table SET a = NEW.a, b = NEW.b WHERE a = OLD.a AND b = OLD.b;
if NOT FOUND then RETURN NULL; end if;
RETURN NEW;
@@ -1030,10 +1034,6 @@ CREATE TRIGGER invalid_trig INSTEAD OF DELETE ON main_table
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
ERROR: "main_table" is a table
DETAIL: Tables cannot have INSTEAD OF triggers.
--- Don't support WHEN clauses with INSTEAD OF triggers
-CREATE TRIGGER invalid_trig INSTEAD OF UPDATE ON main_view
-FOR EACH ROW WHEN (OLD.a <> NEW.a) EXECUTE PROCEDURE view_trigger('instead_of_upd');
-ERROR: INSTEAD OF triggers cannot have WHEN conditions
-- Don't support column-level INSTEAD OF triggers
CREATE TRIGGER invalid_trig INSTEAD OF UPDATE OF a ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
@@ -1049,6 +1049,9 @@ CREATE TRIGGER instead_of_update_trig INSTEAD OF UPDATE ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_upd');
CREATE TRIGGER instead_of_delete_trig INSTEAD OF DELETE ON main_view
FOR EACH ROW EXECUTE PROCEDURE view_trigger('instead_of_del');
+CREATE TRIGGER when_different_update INSTEAD OF UPDATE ON main_view
+FOR EACH ROW WHEN (OLD.a IS DISTINCT FROM NEW.a)
+EXECUTE PROCEDURE view_trigger('instead_of_when');
-- Valid BEFORE statement VIEW triggers
CREATE TRIGGER before_ins_stmt_trig BEFORE INSERT ON main_view
FOR EACH STATEMENT EXECUTE PROCEDURE view_trigger('before_view_ins_stmt');
@@ -1145,18 +1148,47 @@ UPDATE 1
UPDATE main_view SET b = 0 WHERE false;
NOTICE: main_view BEFORE UPDATE STATEMENT (before_view_upd_stmt)
NOTICE: main_view AFTER UPDATE STATEMENT (after_view_upd_stmt)
+UPDATE 0
+-- INSTEAD OF ... WHEN trigger fires.
+UPDATE main_view SET a = 23 WHERE a = 21 RETURNING *;
+NOTICE: main_view BEFORE UPDATE STATEMENT (before_view_upd_stmt)
+NOTICE: main
Greatest Common Divisor
I recently came across the need for a gcd function (actually I needed
lcm) and was surprised that we didn't have one.
So here one is, using the basic Euclidean algorithm. I made one for
smallint, integer, and bigint.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..b2c663cd92 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -870,6 +870,19 @@
-43
+
+
+
+ gcd
+
+gcd(a, b)
+
+ (same as argument types)
+ greatest common divisor
+ gcd(1071, 462)
+ 21
+
+
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 04825fc77d..bc74d367bb 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -1196,6 +1196,47 @@ int2abs(PG_FUNCTION_ARGS)
PG_RETURN_INT16(result);
}
+/* int[24]gcd()
+ * Greatest Common Divisor
+ */
+Datum
+int4gcd(PG_FUNCTION_ARGS)
+{
+ int32 arg1 = PG_GETARG_INT32(0);
+ int32 arg2 = PG_GETARG_INT32(1);
+
+ while (arg2 != 0)
+ {
+ int32 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT32(arg1);
+}
+
+Datum
+int2gcd(PG_FUNCTION_ARGS)
+{
+ int16 arg1 = PG_GETARG_INT16(0);
+ int16 arg2 = PG_GETARG_INT16(1);
+
+ while (arg2 != 0)
+ {
+ int16 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT16(arg1);
+}
+
Datum
int2larger(PG_FUNCTION_ARGS)
{
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index a40ae40dff..97cbd1443b 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -667,6 +667,28 @@ int8mod(PG_FUNCTION_ARGS)
PG_RETURN_INT64(arg1 % arg2);
}
+/* int8gcd()
+ * Greatest Common Divisor
+ */
+Datum
+int8gcd(PG_FUNCTION_ARGS)
+{
+ int64 arg1 = PG_GETARG_INT64(0);
+ int64 arg2 = PG_GETARG_INT64(1);
+
+ while (arg2 != 0)
+ {
+ int64 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT64(arg1);
+}
+
Datum
int8inc(PG_FUNCTION_ARGS)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ac8f64b219..b3057f2cdd 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10729,4 +10729,15 @@
proname => 'pg_partition_root', prorettype => 'regclass',
proargtypes => 'regclass', prosrc => 'pg_partition_root' },
+# greatest common divisor
+{ oid => '8463', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int2', proargtypes => 'int2 int2',
+ prosrc => 'int2gcd' },
+{ oid => '8464', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int4', proargtypes => 'int4 int4',
+ prosrc => 'int4gcd' },
+{ oid => '8465', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int8', proargtypes => 'int8 int8',
+ prosrc => 'int8gcd' },
+
]
diff --git a/src/test/regress/expected/int2.out b/src/test/regress/expected/int2.out
index 8c255b9e4d..66906a7b03 100644
--- a/src/test/regress/expected/int2.out
+++ b/src/test/regress/expected/int2.out
@@ -306,3 +306,11 @@ FROM (VALUES (-2.5::numeric),
2.5 | 3
(7 rows)
+-- gcd
+SELECT gcd(a, b), gcd(a, -b), gcd(-a, b), gcd(-a, -b)
+FROM (VALUES (24948, 4914)) AS v(a, b);
+ gcd | gcd | gcd | gcd
+-+-+-+-
+ 378 | 378 | 378 | 378
+(1 row)
+
diff --git a/src/test/regress/expected/int4.out b/src/test/regress/expected/int4.out
index bda7a8daef..a261964454 100644
--- a/src/test/regress/expected/int4.out
+++ b/src/test/regress/expected/int4.out
@@ -403,3 +403,11 @@ FROM (VALUES (-2.5::numeric),
2.5 | 3
(7 rows)
+-- gcd
+SELECT gcd(a, b), gcd(a, -b), gcd(-a, b), gcd(-a, -b)
+FROM (VALUES (6186, 6410818)) AS v(a, b);
+ gcd | gcd | gcd | gcd
+--+--+--+--
+ 1466 | 1466 | 1466 | 1466
+(1 row)
+
diff --git a/src/test/regress/expected/int8.out b/src/test/regress/expected/int8.out
index 8447a28c3d..1a0836998a 100644
--- a/src/test/regress/expected/int8.out
+++ b/src/test/regress/expected/int8.out
@@ -388,6 +388,16 @@ SELECT '' AS five, q1 * 2 AS "twice int4" FROM INT8_TBL;
| 9135780246913578
(5 rows)
+SELECT q1, q2, gcd(q1, q2) FROM INT8_TBL;
+q1|q2 | gcd
+--+---+--
+ 123 | 456 |3
+ 123 | 4567890123456789 |3
+ 4567890123456789 | 123 |3
+ 4567890123456789 | 4567890123456789 | 4567890123456789
+ 4567890123456789 | -4567890123456789 | 4567890123456789
+(5 rows)
+
-- int8 op int4
SELECT q1 + 42::int4 AS "8plus4", q1 - 42::int4 AS "8minus4", q1 * 42::int4 AS "8mul4", q1 / 42::int4 AS "8div4" FROM INT8_TBL;
Re: ALTER TABLE support for dropping generation expression
On 2019-12-25 12:01, Sergei Kornilov wrote:
Patch does not apply to master. Could you rebase?
done
I noticed one bug:
create table testdrop (i int, b int, m int GENERATED ALWAYS AS ( i*2) stored);
insert into testdrop(i,b) values (3,4);
alter table testdrop alter COLUMN m drop expression ;
alter table testdrop drop column i;
Here is no "m" column anymore. Possible due some forgotten dependency?
fixed -- good catch
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 465312aafd88325a48d934a2c64ca6b9d12dea3f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 28 Dec 2019 17:38:12 +0100
Subject: [PATCH v2] ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION
Add an ALTER TABLE subcommand for dropping the generated property from
a column, per SQL standard.
Discussion:
https://www.postgresql.org/message-id/flat/2f7f1d9c-946e-0453-d841-4f38eb9d69b6%402ndquadrant.com
---
doc/src/sgml/ref/alter_table.sgml | 18
src/backend/catalog/sql_features.txt| 2 +-
src/backend/commands/tablecmds.c| 131 +++-
src/backend/parser/gram.y | 20 +++-
src/bin/psql/tab-complete.c | 2 +-
src/include/nodes/parsenodes.h | 1 +
src/include/parser/kwlist.h | 1 +
src/test/regress/expected/generated.out | 80 +++
src/test/regress/sql/generated.sql | 32 ++
9 files changed, 283 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/alter_table.sgml
b/doc/src/sgml/ref/alter_table.sgml
index 8403c797e2..4bf449587c 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -46,6 +46,7 @@
ALTER [ COLUMN ] column_name
SET DEFAULT expression
ALTER [ COLUMN ] column_name
DROP DEFAULT
ALTER [ COLUMN ] column_name
{ SET | DROP } NOT NULL
+ALTER [ COLUMN ] column_name
DROP EXPRESSION [ IF EXISTS ]
ALTER [ COLUMN ] column_name
ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ (
sequence_options ) ]
ALTER [ COLUMN ] column_name
{ SET GENERATED { ALWAYS | BY DEFAULT } | SET
sequence_option | RESTART [ [ WITH ] restart ] } [...]
ALTER [ COLUMN ] column_name
DROP IDENTITY [ IF EXISTS ]
@@ -241,6 +242,23 @@ Description
+
+DROP EXPRESSION [ IF EXISTS ]
+
+
+ This form turns a stored generated column into a normal base column.
+ Existing data in the columns is retained, but future changes will no
+ longer apply the generation expression.
+
+
+
+ If DROP EXPRESSION IF EXISTS is specified and the
+ column is not a stored generated column, no error is thrown. In this
+ case a notice is issued instead.
+
+
+
+
ADD GENERATED { ALWAYS | BY DEFAULT } AS
IDENTITY
SET GENERATED { ALWAYS | BY DEFAULT }
diff --git a/src/backend/catalog/sql_features.txt
b/src/backend/catalog/sql_features.txt
index ab3e381cff..9f840ddfd2 100644
--- a/src/backend/catalog/sql_features.txt
+++ b/src/backend/catalog/sql_features.txt
@@ -252,7 +252,7 @@ F381Extended schema manipulation03 ALTER
TABLE statement: DROP CONSTRAINT clau
F382 Alter column data type YES
F383 Set column not null clause YES
F384 Drop identity property clause YES
-F385 Drop column generation expression clauseNO
+F385 Drop column generation expression clauseYES
F386 Set identity column generation clause YES
F391 Long identifiersYES
F392 Unicode escapes in identifiers YES
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5b882f80bf..603779ae9d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -386,6 +386,7 @@ static ObjectAddress ATExecAddIdentity(Relation rel, const
char *colName,
static ObjectAddress ATExecSetIdentity(Relation rel, const char *colName,
Node
*def, LOCKMODE lockmode);
static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName,
bool missing_ok, LOCKMODE lockmode);
+static ObjectAddress ATExecDropExpression(Relation rel, const char *colName,
bool missing_ok, LOCKMODE lockmode);
static ObjectAddress ATExecSetStatistics(Relation rel, const char *colName,
int16 colNum,
Node *newValue, LOCKMODE lockmode);
static ObjectAddress ATExecSetOptions(Relation rel, const char *colName,
@@ -3669,6 +3670,7 @@ AlterTableGetLockLevel(List *cmds)
case AT_AddIdentity:
case AT_DropIdentity:
case AT_SetIdentity:
+ case AT_DropExpression:
Recognizing superuser in pg_hba.conf
It can sometimes be useful to match against a superuser in pg_hba.conf.
For example, one could imagine wanting to reject nonsuperuser from a
particular database.
This used to be possible by creating an empty role and matching against
that, but that functionality was removed (a long time ago) by commit
94cd0f1ad8a.
Adding another keyword can break backwards compatibility, of course. So
that is an issue that needs to be discussed, but I don't imagine too
many people are using role names "superuser" and "nonsuperuser". Those
who are will have to quote them.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 5f1eec78fb..c0d1e00266 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -250,7 +250,11 @@ hostnogssenc database user
Specifies which database user name(s) this record
matches. The value all specifies that it
- matches all users. Otherwise, this is either the name of a specific
+ matches all users.
+ The value superuser specifies that it matches all
+ superusers. The value nonsuperuser specifies that it
+ matches no superusers.
+ Otherwise, this is either the name of a specific
database user, or a group name preceded by +.
(Recall that there is no real distinction between users and groups
in PostgreSQL; a + mark really means
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index b6de92783a..a7a59a0510 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -596,6 +596,16 @@ check_role(const char *role, Oid roleid, List *tokens)
if (is_member(roleid, tok->string + 1))
return true;
}
+ else if (token_is_keyword(tok, "superuser"))
+ {
+ if (superuser_arg(roleid))
+return true;
+ }
+ else if (token_is_keyword(tok, "nonsuperuser"))
+ {
+ if (!superuser_arg(roleid))
+return true;
+ }
else if (token_matches(tok, role) ||
token_is_keyword(tok, "all"))
return true;
Re: PostgreSQL 12.1 patch for "private_modify" table creation option for data validation reinforcement
Abdul Yadi AH-2 writes: > As I have published on > https://abdulyadi.wordpress.com/2019/12/26/reinforce-data-validation-prevent-direct-table-modification/, > the patch is to have "private_modify" option in table creation. For example: > CREATE TABLE mytable (id integer) WITH (private_modify=true); > Having the option set, even superuser can not insert/update/delete the > table outside SQL or SPI-based function where complex data validation takes > place. I do not actually see the point of this. It seems randomly inconsistent with the normal SQL permissions mechanisms, and it's not very flexible, nor does it add any meaningful security AFAICS. Anybody who can execute SQL can create a function. For that matter, you don't even need to create a persistent function: you can just wrap the command in a DO block, and that'll bypass this restriction. In what way is this better than the usual technique of putting the table update logic into SECURITY DEFINER functions, and then not granting update rights to anybody other than the owner of those functions? (Please don't say "because it blocks superusers too". That's an anti-feature.) I'm also slightly astonished by your choice to tie the implementation to snapshots. If we do accept something like this, we most certainly aren't going to do it like that. regards, tom lane
Re: Recognizing superuser in pg_hba.conf
Vik Fearing writes: > It can sometimes be useful to match against a superuser in pg_hba.conf. Seems like a reasonable desire. > Adding another keyword can break backwards compatibility, of course. So > that is an issue that needs to be discussed, but I don't imagine too > many people are using role names "superuser" and "nonsuperuser". Those > who are will have to quote them. I'm not very happy about the continuing creep of pseudo-reserved database and user names in pg_hba.conf. I wish we'd adjust the notation so that these keywords are syntactically distinct from ordinary names. Given the precedent that "+" and "@" prefixes change what an identifier means, maybe we could use "*" or some other punctuation character as a keyword prefix? We'd have to give grandfather exceptions to the existing keywords, at least for a while, but we could say that new ones won't be recognized without the prefix. regards, tom lane
Re: Greatest Common Divisor
Bonsoir Vik, I recently came across the need for a gcd function (actually I needed lcm) and was surprised that we didn't have one. Why not. So here one is, using the basic Euclidean algorithm. I made one for smallint, integer, and bigint. Should pg provide the LCM as well? Hmmm, probably not, too likely to overflow. Should there be a NUMERIC version as well? I'd say maybe yes. I'm wondering what it should do on N, 0 and 0, 0. Raise an error? Return 0? Return 1? return N? There should be some logic and comments explaining it. I'd test with INT_MIN and INT_MAX. Given that there are no overflows risk with the Euclidian descent, would it make sense that the int2 version call the int4 implementation? C modulo operator (%) is a pain because it is not positive remainder (2 % -3 == -1 vs 2 % 3 == 2, AFAICR). It does not seem that fixing the sign afterwards is the right thing to do. I'd rather turn both arguments positive before doing the descent. Which raises an issue with INT_MIN by the way, which has no positive:-( Also, the usual approach is to exchange args so that the largest is first, because there may be a software emulation if the hardware does not implement modulo. At least it was the case with some sparc processors 25 years ago:-) See for instance (the int min value is probably not well handled): https://svn.cri.ensmp.fr/svn/linear/trunk/src/arithmetique/pgcd.c Basically, welcome to arithmetic:-) -- Fabien.
Re: Recognizing superuser in pg_hba.conf
On 28/12/2019 19:07, Tom Lane wrote: > Vik Fearing writes: >> It can sometimes be useful to match against a superuser in pg_hba.conf. > Seems like a reasonable desire. > >> Adding another keyword can break backwards compatibility, of course. So >> that is an issue that needs to be discussed, but I don't imagine too >> many people are using role names "superuser" and "nonsuperuser". Those >> who are will have to quote them. > I'm not very happy about the continuing creep of pseudo-reserved database > and user names in pg_hba.conf. I wish we'd adjust the notation so that > these keywords are syntactically distinct from ordinary names. Given > the precedent that "+" and "@" prefixes change what an identifier means, > maybe we could use "*" or some other punctuation character as a keyword > prefix? We'd have to give grandfather exceptions to the existing > keywords, at least for a while, but we could say that new ones won't be > recognized without the prefix. I'm all for this (and even suggested it during the IRC conversation that prompted this patch). It's rife with bikeshedding, though. My original proposal was to use '&' and Andrew Gierth would have used ':'. I will submit two patches, one that recognizes the sigil for all the other keywords, and then an update of this patch. -- Vik
TAP testing for psql's tab completion code
We've often talked about the problem that we have no regression test coverage for psql's tab completion code. I got interested in this issue while messing with the filename completion logic therein [1], so here is a draft patch that adds some testing for that code. This is just preliminary investigation, so I've made no attempt to test tab-complete.c comprehensively (and I'm not sure there would be much point in covering every last code path in it anyway). Still, it does get us from zero to 43% coverage of that file already, and it does good things for the coverage of input.c and prompt.c as well. What I think is actually interesting at this stage is portability of the tests. There are a number of issues: * The script requires Perl's IO::Pty module (indirectly, in that IPC::Run requires that to make pty connections), which isn't installed everywhere. I just made the script skip if that's not available, so that we're not moving the goalposts for what has to be installed to run the TAP tests. Is that the right answer? * It seems pretty likely that this won't work on Windows, given all the caveats in the IPC::Run documentation about nonfunctionality of the pty support there. Maybe we don't care, seeing that we don't really support readline on Windows anyway. For the moment I assumed that the skip conditions for --without-readline and/or missing-IO::Pty would cover this, but we might need an explicit check for Windows too. Or maybe somebody wants to try to make it work on Windows; but that won't be me. * What's even more likely to be problematic is small variations in the output behavior of different readline and libedit versions. According to my tests so far, though, all modern versions of them do pass these test cases. I noted failures on very old Apple versions of libedit: 1. macOS 10.5's version of libedit seems not to honor rl_completion_append_character; it never emits the trailing space we're expecting. This seems like a plain old libedit bug, especially since 10.4's version works as expected. 2. Both 10.4 and 10.5 emit the alternative table names in the wrong order, suggesting that they're not internally sorting the completion results. If this proves to be more widespread, we could likely fix it by adding ORDER BY to the completion queries, but I'm not sure that it's worth doing if only these dead macOS versions have the issue. (On the other hand, it seems like bad practice to be issuing queries that have LIMIT without ORDER BY, so maybe we should fix them anyway.) I'm strongly tempted to just push this and see what the buildfarm thinks of it. If it fails in lots of places, maybe the idea is a dead end. If it works, I'd look into extending the tests --- in particular, I'd like to have some coverage for the filename completion logic. Thoughts? regards, tom lane [1] https://www.postgresql.org/message-id/14585.1577486216%40sss.pgh.pa.us diff --git a/configure b/configure index 9de5037..a133f66 100755 --- a/configure +++ b/configure @@ -706,6 +706,7 @@ with_libxml XML2_CONFIG UUID_EXTRA_OBJS with_uuid +with_readline with_systemd with_selinux with_openssl @@ -8000,6 +8001,7 @@ $as_echo "$as_me: WARNING: *** Readline does not work on MinGW --- disabling" >& fi + # # Prefer libedit # diff --git a/configure.in b/configure.in index 9c5e5e7..9172622 100644 --- a/configure.in +++ b/configure.in @@ -875,6 +875,7 @@ if test "$PORTNAME" = "win32"; then with_readline=no fi fi +AC_SUBST(with_readline) # diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 05b6638..5002c47 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -185,6 +185,7 @@ with_perl = @with_perl@ with_python = @with_python@ with_tcl = @with_tcl@ with_openssl = @with_openssl@ +with_readline = @with_readline@ with_selinux = @with_selinux@ with_systemd = @with_systemd@ with_gssapi = @with_gssapi@ diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore index c2862b1..10b6dd3 100644 --- a/src/bin/psql/.gitignore +++ b/src/bin/psql/.gitignore @@ -1,5 +1,5 @@ /psqlscanslash.c /sql_help.h /sql_help.c - /psql +/tmp_check/ diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile index 42771bc..d08767b 100644 --- a/src/bin/psql/Makefile +++ b/src/bin/psql/Makefile @@ -16,6 +16,9 @@ subdir = src/bin/psql top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +# make this available to TAP test scripts +export with_readline + REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS) @@ -73,8 +76,15 @@ uninstall: clean distclean: rm -f psql$(X) $(OBJS) lex.backup + rm -rf tmp_check # files removed here are supposed to be in the distribution tarball, # so do not clean them in the clean/distclean rules maintainer-clean: distclean rm -f sql_help.h sql_help.c psqlscanslash.c + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) diff --git a/src/bin/ps
Incremental View Maintenance: ERROR: out of shared memory
Hello here is an unexpected error found while testing IVM v11 patches create table b1 (id integer, x numeric(10,3)); create incremental materialized view mv1 as select id, count(*),sum(x) from b1 group by id; do $$ declare i integer; begin for i in 1..1 loop insert into b1 values (1,1); end loop; end; $$ ; ERROR: out of shared memory HINT: You might need to increase max_locks_per_transaction. CONTEXT: SQL statement "DROP TABLE pg_temp_3.pg_temp_66154" SQL statement "insert into b1 values (1,1)" PL/pgSQL function inline_code_block line 1 at SQL statement Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: [PATCH v1] pg_ls_tmpdir to show directories
Here's a version which uses an array of directory_fctx, rather than of DIR and
location. That avoids changing the data structure and collatoral implications
to pg_ls_dir().
Currently, this *shows* subdirs of subdirs, but doesn't decend into them.
So I think maybe toplevel subdirs should be shown, too.
And maybe the is_dir flag should be re-introduced (although someone could call
pg_stat_file if needed).
I'm interested to hear feedback on that, although this patch still isn't great.
>From dd3b2779939fc1b396fed1fba2f7cefc9a6b1ad5 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Fri, 27 Dec 2019 23:34:14 -0600
Subject: [PATCH v4 1/2] BUG: in errmsg
Note there's two changes here.
Should backpatch to v12, where pg_ls_tmpdir was added.
---
src/backend/utils/adt/genfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 5d4f26a..c978e15 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -590,7 +590,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok)
if (stat(path, &attrib) < 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not stat directory \"%s\": %m", dir)));
+ errmsg("could not stat file \"%s\": %m", path)));
/* Ignore anything but regular files */
if (!S_ISREG(attrib.st_mode))
--
2.7.4
>From 30031c790fe5bb358f0bb372cb2d7975e2d688aa Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 14 Dec 2019 16:22:15 -0600
Subject: [PATCH v4 2/2] pg_ls_tmpdir to show directories
See also 9cd92d1a33699f86aa53d44ab04cc3eb50c18d11
---
doc/src/sgml/func.sgml | 14 +++--
src/backend/utils/adt/genfile.c | 136 ++--
2 files changed, 97 insertions(+), 53 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5a98158..8abc643 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -21922,12 +21922,14 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
setof record
-List the name, size, and last modification time of files in the
-temporary directory for tablespace. If
-tablespace is not provided, the
-pg_default tablespace is used. Access is granted
-to members of the pg_monitor role and may be
-granted to other non-superuser roles.
+For files in the temporary directory for
+tablespace, list the name, size, and last modification time.
+Files beneath a first-level directory are shown, and include a pathname
+component of their parent directory; such files are used by parallel processes.
+If tablespace is not provided, the
+pg_default tablespace is used. Access is granted to
+members of the pg_monitor role and may be granted to
+other non-superuser roles.
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index c978e15..6bfac64 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -522,12 +522,84 @@ pg_ls_dir_1arg(PG_FUNCTION_ARGS)
return pg_ls_dir(fcinfo);
}
-/* Generic function to return a directory listing of files */
+/* Recursive helper to handle showing a first level of files beneath a subdir */
static Datum
-pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok)
+pg_ls_dir_files_recurse(FunctionCallInfo fcinfo, FuncCallContext *funcctx, const char *dir, bool missing_ok, bool dir_ok)
+{
+ bool nulls[3] = {0,};
+ Datum values[3];
+
+ directory_fctx *fctx = (directory_fctx *) funcctx->user_fctx;
+
+ while (1) {
+ struct dirent *de;
+ char *location;
+ DIR *dirdesc;
+
+ location = fctx[1].location ? fctx[1].location : fctx[0].location;
+ dirdesc = fctx[1].dirdesc ? fctx[1].dirdesc : fctx[0].dirdesc;
+
+ while ((de = ReadDir(dirdesc, location)) != NULL)
+ {
+ char path[MAXPGPATH * 2];
+ HeapTuple tuple;
+ struct stat attrib;
+
+ /* Skip hidden files */
+ if (de->d_name[0] == '.')
+continue;
+
+ /* Get the file info */
+ snprintf(path, sizeof(path), "%s/%s", location, de->d_name);
+ if (stat(path, &attrib) < 0)
+ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", path)));
+
+ /* Ignore anything but regular files or (if requested) dirs */
+ if (S_ISDIR(attrib.st_mode)) {
+/* Note: decend into dirs, but do not return a tuple for the dir itself */
+/* Do not expect dirs more than one level deep */
+if (dir_ok && !fctx[1].location) {
+ MemoryContext oldcontext;
+ oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+ fctx[1].location = pstrdup(path);
+ fctx[1].dirdesc = AllocateDir(path);
+ MemoryContextSwitchTo(oldcontext);
+ return pg_ls_dir_files_recurse(fcinfo, funcctx, path, missing_ok, false);
+}
+/* else: fall through and show the
Re: TAP testing for psql's tab completion code
Hello Tom, We've often talked about the problem that we have no regression test coverage for psql's tab completion code. I got interested in this issue while messing with the filename completion logic therein [1], so here is a draft patch that adds some testing for that code. This is just preliminary investigation, so I've made no attempt to test tab-complete.c comprehensively (and I'm not sure there would be much point in covering every last code path in it anyway). Still, it does get us from zero to 43% coverage of that file already, and it does good things for the coverage of input.c and prompt.c as well. What I think is actually interesting at this stage is portability of the tests. There are a number of issues: * The script requires Perl's IO::Pty module (indirectly, in that IPC::Run requires that to make pty connections), which isn't installed everywhere. I just made the script skip if that's not available, so that we're not moving the goalposts for what has to be installed to run the TAP tests. Is that the right answer? * It seems pretty likely that this won't work on Windows, given all the caveats in the IPC::Run documentation about nonfunctionality of the pty support there. Maybe we don't care, seeing that we don't really support readline on Windows anyway. For the moment I assumed that the skip conditions for --without-readline and/or missing-IO::Pty would cover this, but we might need an explicit check for Windows too. Or maybe somebody wants to try to make it work on Windows; but that won't be me. * What's even more likely to be problematic is small variations in the output behavior of different readline and libedit versions. According to my tests so far, though, all modern versions of them do pass these test cases. I noted failures on very old Apple versions of libedit: 1. macOS 10.5's version of libedit seems not to honor rl_completion_append_character; it never emits the trailing space we're expecting. This seems like a plain old libedit bug, especially since 10.4's version works as expected. 2. Both 10.4 and 10.5 emit the alternative table names in the wrong order, suggesting that they're not internally sorting the completion results. If this proves to be more widespread, we could likely fix it by adding ORDER BY to the completion queries, but I'm not sure that it's worth doing if only these dead macOS versions have the issue. (On the other hand, it seems like bad practice to be issuing queries that have LIMIT without ORDER BY, so maybe we should fix them anyway.) I'm strongly tempted to just push this and see what the buildfarm thinks of it. If it fails in lots of places, maybe the idea is a dead end. If it works, I'd look into extending the tests --- in particular, I'd like to have some coverage for the filename completion logic. Thoughts? After you raised the issue, I submitted something last August, which did not attract much attention. https://commitfest.postgresql.org/26/2262/ It covers some tab-completion stuff. It uses Expect for the interactive stuff (tab completion, \h, ...). -- Fabien.
Re: Disallow cancellation of waiting for synchronous replication
On Fri, Dec 20, 2019 at 12:04 AM Andrey Borodin wrote: > Currently, we can have split brain with the combination of following steps: > 0. Setup cluster with synchronous replication. Isolate primary from standbys. > 1. Issue upsert query INSERT .. ON CONFLICT DO NOTHING > 2. CANCEL 1 during wait for synchronous replication > 3. Retry 1. Idempotent query will succeed and client have confirmation of > written data, while it is not present on any standby. It seems to me that in order for synchronous replication to work reliably, you've got to be very careful about any situation where a commit might or might not have completed, and this is one such situation. When the client sends the query cancel, it does not and cannot know whether the INSERT statement has not yet completed, has already completed but not yet replicated, or has completed and replicated but not yet sent back a response. However, the server's response will be different in each of those cases, because in the second case, there will be a WARNING about synchronous replication having been interrupted. If the client ignores that WARNING, there are going to be problems. Now, even if you do pay attention to the warning, things are not totally great here, because if you have inadvertently interrupted a replication wait, how do you recover? You can't send a command that means "oh, I want to wait after all." You would have to check the standbys yourself, from the application code, and see whether the changes that the query made have shown up there, or check the LSN on the master and wait for the standbys to advance to that LSN. That's not great, but might be doable for some applications. One idea that I had during the initial discussion around synchronous replication was that maybe there ought to be a distinction between trying to cancel the query and trying to cancel the replication wait. Imagine that you could send a cancel that would only cancel replication waits but not queries, or only queries but not replication waits. Then you could solve this problem by asking the server to PQcancelWithAdvancedMagic(conn, PQ_CANCEL_TYPE_QUERY). I wasn't sure that people would want this, and it didn't seem essential for the version of this feature, but maybe this example shows that it would be worthwhile. I don't really have any idea how you'd integrate such a feature with psql, but maybe it would be good enough to have it available through the C interface. Also, it's a wire-protocol change, so there are compatibility issues to think about. All that being said, like Tom and Michael, I don't think teaching the backend to ignore cancels is the right approach. We have had innumerable problems over the years that were caused by the backend failing to respond to cancels when we would really have liked it to do so, and users REALLY hate it when you tell them that they have to shut down and restart (with crash recovery) the entire database because of a single stuck backend. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Greatest Common Divisor
On 28/12/2019 19:15, Fabien COELHO wrote:
>
>> So here one is, using the basic Euclidean algorithm. I made one for
>> smallint, integer, and bigint.
>
> Should pg provide the LCM as well? Hmmm, probably not, too likely to
> overflow.
I decided against it for that reason.
> Should there be a NUMERIC version as well? I'd say maybe yes.
I thought about that, too, but also decided against it for this patch.
> I'm wondering what it should do on N, 0 and 0, 0. Raise an error?
> Return 0? Return 1? return N? There should be some logic and comments
> explaining it.
Well, gcd(N, 0) is N, and gcd(0, 0) is 0, so I don't see an issue here?
> I'd test with INT_MIN and INT_MAX.
Okay, I'll add tests for those, instead of the pretty much random values
I have now.
> Given that there are no overflows risk with the Euclidian descent, would
> it make sense that the int2 version call the int4 implementation?
Meh.
>
> C modulo operator (%) is a pain because it is not positive remainder
> (2 % -3 == -1 vs 2 % 3 == 2, AFAICR).
This does not seem to be the case...
> It does not seem that fixing the sign afterwards is the right thing to
> do. I'd rather turn both arguments positive before doing the descent.
Why isn't it the right thing to do?
> Which raises an issue with INT_MIN by the way, which has no positive:-(
That's an argument against abs-ing the input values. It's only an issue
with gcd(INT_MIN, INT_MIN) which currently returns INT_MIN. Any other
value with INT_MIN will be 1 or something lower than INT_MAX.
> Also, the usual approach is to exchange args so that the largest is
> first, because there may be a software emulation if the hardware does
> not implement modulo. At least it was the case with some sparc
> processors 25 years ago:-)
The args will exchange themselves.
Thanks for the review! Attached is a new patch that changes the
regression tests based on your comments (and another comment that I got
on irc to test gcd(b, a)).
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..b2c663cd92 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -870,6 +870,19 @@
-43
+
+
+
+ gcd
+
+gcd(a, b)
+
+ (same as argument types)
+ greatest common divisor
+ gcd(1071, 462)
+ 21
+
+
diff --git a/src/backend/utils/adt/int.c b/src/backend/utils/adt/int.c
index 04825fc77d..bc74d367bb 100644
--- a/src/backend/utils/adt/int.c
+++ b/src/backend/utils/adt/int.c
@@ -1196,6 +1196,47 @@ int2abs(PG_FUNCTION_ARGS)
PG_RETURN_INT16(result);
}
+/* int[24]gcd()
+ * Greatest Common Divisor
+ */
+Datum
+int4gcd(PG_FUNCTION_ARGS)
+{
+ int32 arg1 = PG_GETARG_INT32(0);
+ int32 arg2 = PG_GETARG_INT32(1);
+
+ while (arg2 != 0)
+ {
+ int32 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT32(arg1);
+}
+
+Datum
+int2gcd(PG_FUNCTION_ARGS)
+{
+ int16 arg1 = PG_GETARG_INT16(0);
+ int16 arg2 = PG_GETARG_INT16(1);
+
+ while (arg2 != 0)
+ {
+ int16 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT16(arg1);
+}
+
Datum
int2larger(PG_FUNCTION_ARGS)
{
diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c
index a40ae40dff..97cbd1443b 100644
--- a/src/backend/utils/adt/int8.c
+++ b/src/backend/utils/adt/int8.c
@@ -667,6 +667,28 @@ int8mod(PG_FUNCTION_ARGS)
PG_RETURN_INT64(arg1 % arg2);
}
+/* int8gcd()
+ * Greatest Common Divisor
+ */
+Datum
+int8gcd(PG_FUNCTION_ARGS)
+{
+ int64 arg1 = PG_GETARG_INT64(0);
+ int64 arg2 = PG_GETARG_INT64(1);
+
+ while (arg2 != 0)
+ {
+ int64 tmp = arg2;
+ arg2 = arg1 % arg2;
+ arg1 = tmp;
+ }
+
+ if (arg1 < 0)
+ arg1 = -arg1;
+
+ PG_RETURN_INT64(arg1);
+}
+
Datum
int8inc(PG_FUNCTION_ARGS)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ac8f64b219..b3057f2cdd 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10729,4 +10729,15 @@
proname => 'pg_partition_root', prorettype => 'regclass',
proargtypes => 'regclass', prosrc => 'pg_partition_root' },
+# greatest common divisor
+{ oid => '8463', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int2', proargtypes => 'int2 int2',
+ prosrc => 'int2gcd' },
+{ oid => '8464', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int4', proargtypes => 'int4 int4',
+ prosrc => 'int4gcd' },
+{ oid => '8465', descr => 'greatest common divisor',
+ proname => 'gcd', prorettype => 'int8', proargtypes => 'int8 int8',
+ prosrc => 'int8gcd' },
+
]
diff --git a/src/test/regress/expected/int2.out b/src/test/regress/expected/int2.out
index 8c255b9e4d..cca47af7fb 100644
--- a/src/test/regress/expected/int2.out
+++ b/src/test/regress/expected/int2.out
@@ -306,3 +306,23 @@ FROM (VALUES (-2.5::numeric),
2.5 | 3
(7 row
Re: use CLZ instruction in AllocSetFreeIndex()
John Naylor writes:
> v2 had an Assert that was only correct while experimenting with
> eliding right shift. Fixed in v3.
I think there must have been something wrong with your test that
said that eliminating the right shift from the non-CLZ code made
it slower. It should be an unconditional win, just as it is for
the CLZ code path. (Maybe some odd cache-line-boundary effect?)
Also, I think it's just weird to account for ALLOC_MINBITS one
way in the CLZ path and the other way in the other path.
I decided that it might be a good idea to do performance testing
in-place rather than in a standalone test program. I whipped up
the attached that just does a bunch of palloc/pfree cycles.
I got the following results on a non-cassert build (medians of
a number of tests; the times are repeatable to ~ 0.1% for me):
HEAD: 2429.431 ms
v3 CLZ: 2131.735 ms
v3 non-CLZ: 2477.835 ms
remove shift: 2266.755 ms
I didn't bother to try this on non-x86_64 architectures, as
previous testing convinces me the outcome should be about the
same.
Hence, pushed that way, with a bit of additional cosmetic foolery:
the static assertion made more sense to me in relation to the
documented assumption that size <= ALLOC_CHUNK_LIMIT, and I
thought the comment could use some work.
regards, tom lane
/*
create function drive_palloc(count int) returns void
strict volatile language c as '.../drive_palloc.so';
\timing
select drive_palloc(1000);
*/
#include "postgres.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
PG_MODULE_MAGIC;
/*
* drive_palloc(count int) returns void
*/
PG_FUNCTION_INFO_V1(drive_palloc);
Datum
drive_palloc(PG_FUNCTION_ARGS)
{
int32 count = PG_GETARG_INT32(0);
while (count-- > 0)
{
for (size_t sz = 1; sz <= 8192; sz <<= 1)
{
void *p = palloc(sz);
pfree(p);
}
CHECK_FOR_INTERRUPTS();
}
PG_RETURN_VOID();
}
Re: TAP testing for psql's tab completion code
Fabien COELHO writes: >> We've often talked about the problem that we have no regression test >> coverage for psql's tab completion code. I got interested in this >> issue while messing with the filename completion logic therein [1], >> so here is a draft patch that adds some testing for that code. > After you raised the issue, I submitted something last August, which did > not attract much attention. >https://commitfest.postgresql.org/26/2262/ > It covers some tab-completion stuff. It uses Expect for the interactive > stuff (tab completion, \h, ...). Now that you mention it, I seem to recall looking at that and not being happy with the additional dependency on Expect. Expect is *not* a standard module; on the machines I have handy, the only one in which it appears in the default Perl installation is macOS. (Huh, what's Apple doing out ahead of the pack?) I'm pretty sure that Expect also relies on IO::Pty, so it's a strictly worse dependency than what I've got here. Can we recast what you did into something like this patch's methods? regards, tom lane
Re: Disallow cancellation of waiting for synchronous replication
On 29.12.2019 00:55, Robert Haas wrote: On Fri, Dec 20, 2019 at 12:04 AM Andrey Borodin wrote: Currently, we can have split brain with the combination of following steps: 0. Setup cluster with synchronous replication. Isolate primary from standbys. 1. Issue upsert query INSERT .. ON CONFLICT DO NOTHING 2. CANCEL 1 during wait for synchronous replication 3. Retry 1. Idempotent query will succeed and client have confirmation of written data, while it is not present on any standby. All that being said, like Tom and Michael, I don't think teaching the backend to ignore cancels is the right approach. We have had innumerable problems over the years that were caused by the backend failing to respond to cancels when we would really have liked it to do so, and users REALLY hate it when you tell them that they have to shut down and restart (with crash recovery) the entire database because of a single stuck backend. The stuckness of backend is not deadlock here. To cancel waiting of backend fluently, client is enough to turn off synchronous replication (change synchronous_standby_names through server reload) or change synchronous replica to another livable one (again through changing of synchronous_standby_names). In first case he explicitly agrees with existence of local (not replicated) commits in master. -- Best regards, Maksim Milyutin
Re: xact_start for walsender & logical decoding not updated
On Fri, Dec 27, 2019 at 04:46:18PM -0300, Alvaro Herrera wrote: On 2019-Dec-13, Kyotaro Horiguchi wrote: At Fri, 13 Dec 2019 13:05:41 +0800, Craig Ringer wrote in > On Wed, 11 Dec 2019 at 02:08, Alvaro Herrera > wrote: > > > On 2019-Dec-10, Tomas Vondra wrote: > > > > > On Tue, Dec 10, 2019 at 09:42:17AM +0900, Kyotaro Horiguchi wrote: > > > > At Tue, 10 Dec 2019 00:44:09 +0100, Tomas Vondra < > > tomas.von...@2ndquadrant.com> wrote in > > > > > > I'm not sure how much xact_start for walsender is useful and we really > > > > is not running a statement there. Also autovac launcher starts > > > > transaction without a valid statement timestamp perhaps for the same > > > > reason. > > > > > > Maybe, but then maybe we should change it so that we don't report any > > > timestamps for such processes. > > > > Yeah, I think we should to that. > Agreed. Don't report a transaction start timestamp at all if we're not in a > read/write txn in the walsender, which we should never be when using a > historic snapshot. > > It's not interesting or relevant. This patch changes xact.c to avoid updating transaction start timestamps for walsenders (maybe more commentary is desirable). I think logical decoding is just a special form of walsender and thus it would also be updated by this patch, unless I misunderstood what Tomas explained. It's true walsender should not be doing any read-write transactions or executing statements (well, maybe a decoding plugin could, but using historic snapshot). So I agree not leaving xact_start for walsender processes seems OK. > Reporting the commit timestamp of the current or last-processed xact would > likely just be confusing. I'd rather see that in pg_stat_replication if > we're going to show it, that way we can label it usefully. Sounds reasonable. Developers interested in this feature can submit a patch, as usual :-) ;-) regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Disallow cancellation of waiting for synchronous replication
On Sat, Dec 28, 2019 at 6:19 PM Maksim Milyutin wrote: > The stuckness of backend is not deadlock here. To cancel waiting of > backend fluently, client is enough to turn off synchronous replication > (change synchronous_standby_names through server reload) or change > synchronous replica to another livable one (again through changing of > synchronous_standby_names). In first case he explicitly agrees with > existence of local (not replicated) commits in master. Sure, that's true. But I still maintain that responding to ^C is an important property of the system. If you have to do some more complicated set of steps like the ones you propose here, a decent number of people aren't going to figure it out and will end up unhappy. Now, as it is, you're unhappy, so I guess you can't please everyone, but you asked for opinions so I'm giving you mine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Building infrastructure for B-Tree deduplication that recognizes when opclass equality is also equivalence
On Tue, Dec 24, 2019 at 7:29 AM Anastasia Lubennikova wrote: > We can make this 'opcisbitwise' parameter enum (or char) instead of > boolean to mark > "always bitwise", "never bitwise" and "maybe bitwise". Though, I doubt > if it will be helpful in any real use case. What would be the difference between "never bitwise" and "maybe bitwise" in that scheme? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: error context for vacuum to include block number
On Thu, Dec 26, 2019 at 10:57 AM Justin Pryzby wrote: > I agree that's better. > I don't see any reason why the progress params need to be updated atomically. > So rebasified against your patch. I am not sure whether it's important enough to make a stink about, but it bothers me a bit that this is being dismissed as unimportant. The problem is that, if the updates are not atomic, then somebody might see the data after one has been updated and the other has not yet been updated. The result is that when the phase is PROGRESS_VACUUM_PHASE_VACUUM_INDEX, someone reading the information can't tell whether the number of index scans reported is the number *previously* performed or the number performed including the one that just finished. The race to see the latter state is narrow, so it probably wouldn't come up often, but it does seem like it would be confusing if it did happen. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Consolidate 'unique array values' logic into a reusable function?
On Mon, Nov 04, 2019 at 12:02:21PM +1300, Thomas Munro wrote:
> Rebased. I'm planning to commit this soon.
In each installcheck-parallel run under valgrind-3.14.0, I now see ~1200
reports like this:
==00:00:00:28.322 1527557== Source and destination overlap in memcpy(0x1000104,
0x1000104, 4)
==00:00:00:28.322 1527557==at 0x4C2E74D: memcpy@@GLIBC_2.14
(vg_replace_strmem.c:1035)
==00:00:00:28.322 1527557==by 0xA9A57B: qunique (qunique.h:34)
==00:00:00:28.322 1527557==by 0xA9A843: InitCatalogCache (syscache.c:1056)
==00:00:00:28.322 1527557==by 0xAB6B18: InitPostgres (postinit.c:682)
==00:00:00:28.322 1527557==by 0x91F98E: PostgresMain (postgres.c:3909)
==00:00:00:28.322 1527557==by 0x872DE9: BackendRun (postmaster.c:4498)
==00:00:00:28.322 1527557==by 0x8725B3: BackendStartup (postmaster.c:4189)
==00:00:00:28.322 1527557==by 0x86E7F4: ServerLoop (postmaster.c:1727)
==00:00:00:28.322 1527557==by 0x86E0AA: PostmasterMain (postmaster.c:1400)
==00:00:00:28.322 1527557==by 0x77CB56: main (main.c:210)
==00:00:00:28.322 1527557==
{
Memcheck:Overlap
fun:memcpy@@GLIBC_2.14
fun:qunique
fun:InitCatalogCache
fun:InitPostgres
fun:PostgresMain
fun:BackendRun
fun:BackendStartup
fun:ServerLoop
fun:PostmasterMain
fun:main
}
This is like the problem fixed in 9a9473f; the precedent from there would be
to test src!=dst before calling mempcy(), e.g. as attached. I suppose the
alternative would be to add a suppression like the one 9a9473f removed.
I do wonder why the Valgrind buildfarm animals haven't noticed.
diff --git a/src/include/lib/qunique.h b/src/include/lib/qunique.h
index 4d620f8..fc539ca 100644
--- a/src/include/lib/qunique.h
+++ b/src/include/lib/qunique.h
@@ -30,8 +30,9 @@ qunique(void *array, size_t elements, size_t width,
for (i = 1, j = 0; i < elements; ++i)
{
- if (compare(bytes + i * width, bytes + j * width) != 0)
- memcpy(bytes + ++j * width, bytes + i * width, width);
+ if (compare(bytes + i * width, bytes + j * width) != 0 &&
+ ++j != i)
+ memcpy(bytes + j * width, bytes + i * width, width);
}
return j + 1;
@@ -55,8 +56,9 @@ qunique_arg(void *array, size_t elements, size_t width,
for (i = 1, j = 0; i < elements; ++i)
{
- if (compare(bytes + i * width, bytes + j * width, arg) != 0)
- memcpy(bytes + ++j * width, bytes + i * width, width);
+ if (compare(bytes + i * width, bytes + j * width, arg) != 0 &&
+ ++j != i)
+ memcpy(bytes + j * width, bytes + i * width, width);
}
return j + 1;
Re: TAP testing for psql's tab completion code
Hello Tom, We've often talked about the problem that we have no regression test coverage for psql's tab completion code. I got interested in this issue while messing with the filename completion logic therein [1], so here is a draft patch that adds some testing for that code. After you raised the issue, I submitted something last August, which did not attract much attention. https://commitfest.postgresql.org/26/2262/ It covers some tab-completion stuff. It uses Expect for the interactive stuff (tab completion, \h, ...). Now that you mention it, I seem to recall looking at that and not being happy with the additional dependency on Expect. Possibly. You did not say it out very loud. Expect is *not* a standard module; Somehow. It is an old one, though. on the machines I have handy, the only one in which it appears in the default Perl installation is macOS. (Huh, what's Apple doing out ahead of the pack?) I'm pretty sure that Expect also relies on IO::Pty, Indeed, it does. so it's a strictly worse dependency than what I've got here. If you have to install IO::Pty anyway, ISTM you can also install Expect. IO::Pty documentation says that it is "mainly used by Expect", which is a clue that IO::Pty is not much better than Expect as a dependency. For installation, "apt install libexpect-perl" did the trick for me. "cpan install Expect" should work as well on most setup. I guess it is possible to check whether Expect is available and to skip the corresponding tests if not. Can we recast what you did into something like this patch's methods? Basically it means reimplementing some expect functionality in the script, including new bugs. Modules were invented to avert that, so I cannot say I'm happy with the prospect of re-inventing the wheel. Note that Expect is a pure-perl 1600-LOC module. Anyway, I'll have a look. At least I used a very limited subset of Expect capabilities which should help matters along. -- Fabien.
Re: Greatest Common Divisor
Bonjour Vik, Should there be a NUMERIC version as well? I'd say maybe yes. I thought about that, too, but also decided against it for this patch. Hmmm. ISTM that int functions are available for numeric? I'm wondering what it should do on N, 0 and 0, 0. Raise an error? Return 0? Return 1? return N? There should be some logic and comments explaining it. Well, gcd(N, 0) is N, and gcd(0, 0) is 0, so I don't see an issue here? I think that there should be a comment. I'd test with INT_MIN and INT_MAX. Okay, I'll add tests for those, instead of the pretty much random values I have now. C modulo operator (%) is a pain because it is not positive remainder (2 % -3 == -1 vs 2 % 3 == 2, AFAICR). This does not seem to be the case... Indeed, I tested quickly with python, but it has yet another behavior as shown above, what a laugh! So with C: 2 % -3 == 2, -2 % 3 == -2 Note that AFAICS there is no integer i so that 3 * i - (-2) == -2. It does not seem that fixing the sign afterwards is the right thing to do. I'd rather turn both arguments positive before doing the descent. Why isn't it the right thing to do? Because I do not trust C modulo as I had a lot of problems with it? :-) If it works, but it should deserve a clear comment explaining why. Which raises an issue with INT_MIN by the way, which has no positive:-( That's an argument against abs-ing the input values. It's only an issue with gcd(INT_MIN, INT_MIN) which currently returns INT_MIN. That should be an error instead, because -INT_MIN cannot be represented? Any other value with INT_MIN will be 1 or something lower than INT_MAX. Looks ok. Also, the usual approach is to exchange args so that the largest is first, because there may be a software emulation if the hardware does not implement modulo. At least it was the case with some sparc processors 25 years ago:-) The args will exchange themselves. Yep, but after a possibly expensive software-emulated modulo operation? -- Fabien.
