Re: Extend ALTER DEFAULT PRIVILEGES for large objects
On Wed, 24 Apr 2024 16:08:39 -0500 Nathan Bossart wrote: > On Tue, Apr 23, 2024 at 11:47:38PM -0400, Tom Lane wrote: > > On the whole I find this proposed feature pretty unexciting > > and dubiously worthy of the implementation/maintenance effort. > > I don't have any particularly strong feelings on $SUBJECT, but I'll admit > I'd be much more interested in resolving any remaining reasons folks are > using large objects over TOAST. I see a couple of reasons listed in the > docs [0] that might be worth examining. > > [0] https://www.postgresql.org/docs/devel/lo-intro.html If we could replace large objects with BYTEA in any use cases, large objects would be completely obsolete. However, currently some users use large objects in fact, so improvement in this feature seems beneficial for them. Apart from that, extending TOAST to support more than 1GB data and stream-style access seems a good challenge. I don't know if there was a proposal for this in past. This is just a thought, for this purpose, we will need a new type of varlena that can contains large size information, and a new toast table schema that can store offset information or some way to convert a offset to chunk_seq. Regards, Yugo Nagata > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: Remove unnecessary code rom be_lo_put()
On Thu, 25 Apr 2024 10:26:41 +0200 Peter Eisentraut wrote: > On 24.04.24 15:25, Tom Lane wrote: > > Peter Eisentraut writes: > >> On 24.04.24 11:59, Yugo NAGATA wrote: > >>> I noticed that a permission check is performed in be_lo_put() > >>> just after returning inv_open(), but teh permission should be > >>> already checked in inv_open(), so I think we can remove this > >>> part of codes. I attached a patch for this fix. > > > >> Yes, I think you are right. > > > > I agree. Do you want to do the honors? > > done > Thank you! Regards, Yugo Nagata -- Yugo NAGATA
Re: Rename libpq trace internal functions
On Wed, 24 Apr 2024 09:39:02 +0200 Peter Eisentraut wrote: > libpq's pqTraceOutputMessage() used to look like this: > > case 'Z': /* Ready For Query */ > pqTraceOutputZ(conn->Pfdebug, message, ); > break; > > Commit f4b54e1ed98 introduced macros for protocol characters, so now > it looks like this: > > case PqMsg_ReadyForQuery: > pqTraceOutputZ(conn->Pfdebug, message, ); > break; > > But this introduced a disconnect between the symbol in the switch case > and the function name to be called, so this made the manageability of > this file a bit worse. > > This patch changes the function names to match, so now it looks like > this: > > case PqMsg_ReadyForQuery: > pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, ); > break; +1 I prefer the new function names since it seems more natural and easier to read. I noticed pqTraceOutputNR() is left as is, is this intentional? Or, shoud this be changed to pqTranceOutput_NoticeResponse()? Regards, Yugo Nagata > (This also improves the readability of the file in general, since some > function names like "pqTraceOutputt" were a little hard to read > accurately.) > > Some protocol characters have different meanings to and from the > server. The old code structure had a common function for both, for > example, pqTraceOutputD(). The new structure splits this up into > separate ones to match the protocol message name, like > pqTraceOutput_Describe() and pqTraceOutput_DataRow(). -- Yugo NAGATA
Remove unnecessary code rom be_lo_put()
Hi,
I noticed that a permission check is performed in be_lo_put()
just after returning inv_open(), but teh permission should be
already checked in inv_open(), so I think we can remove this
part of codes. I attached a patch for this fix.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 28ad1c9277..f728d0346c 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -860,17 +860,6 @@ be_lo_put(PG_FUNCTION_ARGS)
lo_cleanup_needed = true;
loDesc = inv_open(loOid, INV_WRITE, CurrentMemoryContext);
- /* Permission check */
- if (!lo_compat_privileges &&
- pg_largeobject_aclcheck_snapshot(loDesc->id,
- GetUserId(),
- ACL_UPDATE,
- loDesc->snapshot) != ACLCHECK_OK)
- ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied for large object %u",
- loDesc->id)));
-
inv_seek(loDesc, offset, SEEK_SET);
written = inv_write(loDesc, VARDATA_ANY(str), VARSIZE_ANY_EXHDR(str));
Assert(written == VARSIZE_ANY_EXHDR(str));
Re: minor error message inconsistency in make_pathkey_from_sortinfo
On Wed, 24 Apr 2024 15:05:00 +0800
jian he wrote:
> hi.
>
> in make_pathkey_from_sortinfo
>
> equality_op = get_opfamily_member(opfamily,
> opcintype,
> opcintype,
> BTEqualStrategyNumber);
> if (!OidIsValid(equality_op)) /* shouldn't happen */
> elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
> BTEqualStrategyNumber, opcintype, opcintype, opfamily);
>
> the error message seems not right?
This message was introduced by 278cb434110 which was aiming to
standardize the wording for similar errors. We can find the pattern
"missing {support function | operator} %d(%u,%u) in opfamily %u"
in several places.
Regards,
Yugo Nagata
>
> maybe
> if (!OidIsValid(equality_op)) /* shouldn't happen */
> elog(ERROR, "missing operator =(%u,%u) in opfamily %u",opcintype,
> opcintype, opfamily);
>
> or
>
> if (!OidIsValid(equality_op)) /* shouldn't happen */
> elog(ERROR, "missing equality operator for type %u in opfamily
> %u",opcintype, opcintype, opfamily);
>
>
--
Yugo NAGATA
Small filx on the documentation of ALTER DEFAULT PRIVILEGES
Hi,
Hi,
We can specify more than one privilege type in
"ALTER DEFAULT PRIVILEGES GRANT/REVOKE ON SCHEMAS",
for example,
ALTER DEFAULT PRIVILEGES GRANT USAGE,CREATE ON SCHEMAS TO PUBLIC;
However, the syntax described in the documentation looks to
be allowing only one,
GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
ON SCHEMAS
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
while the syntaxes for tables and sequences are described correctly.
e.g.
GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
[, ...] | ALL [ PRIVILEGES ] }
ON TABLES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
I attached a small patch to fix the description.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 1de4c5c1b4..89aacec4fa 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -46,7 +46,8 @@ GRANT { USAGE | ALL [ PRIVILEGES ] }
ON TYPES
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
-GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
+GRANT { { USAGE | CREATE }
+[, ...] | ALL [ PRIVILEGES ] }
ON SCHEMAS
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
@@ -77,7 +78,8 @@ REVOKE [ GRANT OPTION FOR ]
[ CASCADE | RESTRICT ]
REVOKE [ GRANT OPTION FOR ]
-{ USAGE | CREATE | ALL [ PRIVILEGES ] }
+{ { USAGE | CREATE }
+[, ...] | ALL [ PRIVILEGES ] }
ON SCHEMAS
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
Re: Extend ALTER DEFAULT PRIVILEGES for large objects
On Tue, 23 Apr 2024 23:47:38 -0400
Tom Lane wrote:
> Yugo NAGATA writes:
> > Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
> > so if we want to allow users other than the owner to use the large
> > object, we need to grant a privilege on it every time a large object
> > is created. One of our clients feels that this is annoying, so I would
> > like propose to extend ALTER DEFAULT PRIVILEGE to large objects.
>
> I wonder how this plays with pg_dump, and in particular whether it
> breaks the optimizations that a45c78e32 installed for large numbers
> of large objects. The added test cases seem to go out of their way
> to leave no trace behind that the pg_dump/pg_upgrade tests might
> encounter.
Thank you for your comments.
The previous patch did not work with pg_dump since I forgot some fixes.
I attached a updated patch including fixes.
I believe a45c78e32 is about already-existing large objects and does
not directly related to default privileges, so will not be affected
by this patch.
> I think you broke psql's \ddp, too. And some other places; grepping
> for DEFACLOBJ_NAMESPACE finds other oversights.
Yes, I did. The attached patch include fixes for psql, too.
> On the whole I find this proposed feature pretty unexciting
> and dubiously worthy of the implementation/maintenance effort.
I believe this feature is beneficial to some users allows because
this enables to omit GRANT that was necessary every large object
creation. It seems to me that implementation/maintenance cost is not
so high compared to other objects (e.g. default privileges on schemas)
unless I am still missing something wrong.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 0cfcdc2b297556248cfb64d67779d5fcb8dab227 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH v2] Extend ALTER DEFAULT PRIVILEGES for large objects
Original patch by Haruka Takatsuka, some fixes and tests by
Yugo Nagata.
---
doc/src/sgml/catalogs.sgml| 3 +-
.../sgml/ref/alter_default_privileges.sgml| 15 ++-
src/backend/catalog/aclchk.c | 21
src/backend/catalog/objectaddress.c | 18 ++-
src/backend/catalog/pg_largeobject.c | 18 ++-
src/backend/parser/gram.y | 5 +-
src/bin/pg_dump/dumputils.c | 3 +-
src/bin/pg_dump/pg_dump.c | 3 +
src/bin/psql/describe.c | 6 +-
src/bin/psql/tab-complete.c | 2 +-
src/include/catalog/pg_default_acl.h | 1 +
src/include/parser/kwlist.h | 1 +
src/test/regress/expected/privileges.out | 104 +-
src/test/regress/sql/privileges.sql | 47
14 files changed, 235 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..b8cc822aeb 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3330,7 +3330,8 @@ SCRAM-SHA-256$iteration count:
S = sequence,
f = function,
T = type,
- n = schema
+ n = schema,
+ L = large object
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 1de4c5c1b4..3b358b7a88 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -50,6 +50,11 @@ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
ON SCHEMAS
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+GRANT { { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
@@ -81,6 +86,13 @@ REVOKE [ GRANT OPTION FOR ]
ON SCHEMAS
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+{ { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+[ CASCADE | RESTRICT ]
@@ -159,7 +171,8 @@ REVOKE [ GRANT OPTION FOR ]
If IN SCHEMA is omitted, the global default privileges
are altered.
IN SCHEMA is not allowed when setting privileges
- for schemas, since schemas can't be nested.
+ for schemas and large objects, since schemas can't be nested and
+ large objects don't belong to a schema.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..41baf81a1d 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1077,6 +1077,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
a
Extend ALTER DEFAULT PRIVILEGES for large objects
Hi,
Currently, ALTER DEFAULT PRIVILEGE doesn't support large objects,
so if we want to allow users other than the owner to use the large
object, we need to grant a privilege on it every time a large object
is created. One of our clients feels that this is annoying, so I would
like propose to extend ALTER DEFAULT PRIVILEGE to large objects.
Here are the new actions allowed in abbreviated_grant_or_revoke;
+GRANT { { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+REVOKE [ GRANT OPTION FOR ]
+{ { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+[ CASCADE | RESTRICT ]
A new keyword OBJECTS is introduced for using plural form in the syntax
as other supported objects. A schema name is not allowed to be specified
for large objects since any large objects don't belong to a schema.
The attached patch is originally proposed by Haruka Takatsuka
and some fixes and tests are made by me.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From fe2cb39bd83d09a052d5d63889acd0968c1817b6 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Fri, 8 Mar 2024 17:43:43 +0900
Subject: [PATCH] Extend ALTER DEFAULT PRIVILEGES for large objects
Original patch by Haruka Takatsuka, some fixes and tests by
Yugo Nagata.
---
doc/src/sgml/catalogs.sgml| 3 +-
.../sgml/ref/alter_default_privileges.sgml| 15 ++-
src/backend/catalog/aclchk.c | 21
src/backend/catalog/pg_largeobject.c | 18 ++-
src/backend/parser/gram.y | 5 +-
src/include/catalog/pg_default_acl.h | 1 +
src/include/parser/kwlist.h | 1 +
src/test/regress/expected/privileges.out | 104 +-
src/test/regress/sql/privileges.sql | 47
9 files changed, 208 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..b8cc822aeb 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -3330,7 +3330,8 @@ SCRAM-SHA-256$iteration count:
S = sequence,
f = function,
T = type,
- n = schema
+ n = schema,
+ L = large object
diff --git a/doc/src/sgml/ref/alter_default_privileges.sgml b/doc/src/sgml/ref/alter_default_privileges.sgml
index 1de4c5c1b4..3b358b7a88 100644
--- a/doc/src/sgml/ref/alter_default_privileges.sgml
+++ b/doc/src/sgml/ref/alter_default_privileges.sgml
@@ -50,6 +50,11 @@ GRANT { USAGE | CREATE | ALL [ PRIVILEGES ] }
ON SCHEMAS
TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+GRANT { { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+TO { [ GROUP ] role_name | PUBLIC } [, ...] [ WITH GRANT OPTION ]
+
REVOKE [ GRANT OPTION FOR ]
{ { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER | MAINTAIN }
[, ...] | ALL [ PRIVILEGES ] }
@@ -81,6 +86,13 @@ REVOKE [ GRANT OPTION FOR ]
ON SCHEMAS
FROM { [ GROUP ] role_name | PUBLIC } [, ...]
[ CASCADE | RESTRICT ]
+
+REVOKE [ GRANT OPTION FOR ]
+{ { SELECT | UPDATE }
+[, ...] | ALL [ PRIVILEGES ] }
+ON LARGE OBJECTS
+FROM { [ GROUP ] role_name | PUBLIC } [, ...]
+[ CASCADE | RESTRICT ]
@@ -159,7 +171,8 @@ REVOKE [ GRANT OPTION FOR ]
If IN SCHEMA is omitted, the global default privileges
are altered.
IN SCHEMA is not allowed when setting privileges
- for schemas, since schemas can't be nested.
+ for schemas and large objects, since schemas can't be nested and
+ large objects don't belong to a schema.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..41baf81a1d 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -1077,6 +1077,10 @@ ExecAlterDefaultPrivilegesStmt(ParseState *pstate, AlterDefaultPrivilegesStmt *s
all_privileges = ACL_ALL_RIGHTS_SCHEMA;
errormsg = gettext_noop("invalid privilege type %s for schema");
break;
+ case OBJECT_LARGEOBJECT:
+ all_privileges = ACL_ALL_RIGHTS_LARGEOBJECT;
+ errormsg = gettext_noop("invalid privilege type %s for large object");
+ break;
default:
elog(ERROR, "unrecognized GrantStmt.objtype: %d",
(int) action->objtype);
@@ -1268,6 +1272,16 @@ SetDefaultACL(InternalDefaultACL *iacls)
this_privileges = ACL_ALL_RIGHTS_SCHEMA;
break;
+ case OBJECT_LARGEOBJECT:
+ if (OidIsValid(iacls->nspid))
+ereport(ERROR,
+ (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg("cannot use IN SCHEMA clause when using GRANT/REVOKE ON LARGE OBJECTS")));
+ objtype = DEFACLOBJ_LARGEOBJECT;
+ if (iacls->all_privs && this_privileges == ACL_NO_RIGHTS)
+this_privileges = ACL_ALL_RIGHTS_LARGEOB
Re: pgbnech: allow to cancel queries during benchmark
On Sat, 30 Mar 2024 14:35:37 +0100 Alvaro Herrera wrote: > Due to commit 61461a300c1c, this patch needs to be reworked. Thank you for pointing out this. Although cfbot doesn't report any failure, but PQcancel is now deprecated and insecure. I'll consider it too while fixing a problem I found in [1]. [1] https://www.postgresql.org/message-id/20240207101903.b5846c25808f64a91ee2e7a2%40sraoss.co.jp Regards, Yugo Nagata > -- > Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ > "La fuerza no está en los medios físicos > sino que reside en una voluntad indomable" (Gandhi) -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 14 Mar 2024 11:10:42 -0500 Nathan Bossart wrote: > On Wed, Mar 13, 2024 at 01:09:18PM +0700, Yugo NAGATA wrote: > > On Tue, 12 Mar 2024 22:07:17 -0500 > > Nathan Bossart wrote: > >> I did some light editing to prepare this for commit. > > > > Thank you. I confirmed the test you improved and I am fine with that. > > Committed. Thank you! > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 12 Mar 2024 22:07:17 -0500 Nathan Bossart wrote: > I did some light editing to prepare this for commit. Thank you. I confirmed the test you improved and I am fine with that. Regards, Yugo Nagata > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Sat, 9 Mar 2024 08:50:28 -0600
Nathan Bossart wrote:
> On Sat, Mar 09, 2024 at 11:57:18AM +0900, Yugo NAGATA wrote:
> > On Fri, 8 Mar 2024 16:17:58 -0600
> > Nathan Bossart wrote:
> >> Is this guaranteed to be TOASTed for all possible page sizes?
> >
> > Should we use block_size?
> >
> > SHOW block_size \gset
> > INSERT INTO test_chunk_id(v1,v2)
> > VALUES (repeat('x', 1), repeat('x', (:block_size / 4)));
> >
> > I think this will work in various page sizes.
>
> WFM
>
> > +SHOW block_size; \gset
> > + block_size
> > +
> > + 8192
> > +(1 row)
>
> I think we need to remove the ';' so that the output of the query is not
> saved in the ".out" file. With that change, this test passes when Postgres
> is built with --with-blocksize=32. However, many other unrelated tests
> begin failing, so I guess this fix isn't tremendously important.
I rewrote the patch to use current_setting('block_size') instead of SHOW
and \gset as other tests do. Although some tests are failing with block_size=32,
I wonder it is a bit better to use "block_size" instead of the constant
to make the test more general to some extent.
Regards,
Yugo Nagata
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From 5b3be1ca6f8d8bafc1d9ce7bea252f364c9c09a9 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v9] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 291ed876fc..443ca854a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7447,6 +7447,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chun
Re: Remove unnecessary code from psql's watch command
On Fri, 08 Mar 2024 12:09:12 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > On Wed, 06 Mar 2024 13:03:39 -0500 > > Tom Lane wrote: > >> I don't have Windows here to test on, but does the WIN32 code > >> path work at all? > > > The outer loop is eventually exited even because PSQLexecWatch returns 0 > > when cancel_pressed = 0. However, it happens after executing an extra > > query in this function not just after exit of the inner loop. Therefore, > > it would be better to adding set and check of "done" in WIN32, too. > > Ah, I see now. Agreed, that could stand improvement. > > > I've attached the updated patch > > (v2_remove_unnecessary_code_in_psql_watch.patch). > > Pushed with minor tidying. Thanks! -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Fri, 8 Mar 2024 16:17:58 -0600
Nathan Bossart wrote:
> On Fri, Mar 08, 2024 at 03:31:55PM +0900, Yugo NAGATA wrote:
> > On Thu, 7 Mar 2024 16:56:17 -0600
> > Nathan Bossart wrote:
> >> to me. Do you think it's worth adding something like a
> >> pg_column_toast_num_chunks() function that returns the number of chunks for
> >> the TOASTed value, too?
> >
> > If we want to know the number of chunks of a specified chunk_id,
> > we can get this by the following query.
> >
> > postgres=# SELECT id, (SELECT count(*) FROM pg_toast.pg_toast_16384 WHERE
> > chunk_id = id)
> > FROM (SELECT pg_column_toast_chunk_id(v) AS id FROM t);
>
> Good point. Overall, I think this patch is in decent shape, so I'll aim to
> commit it sometime next week.
Thank you.
>
> > +{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value',
> > + proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype =>
> > 'oid',
> > + proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
>
> Note to self: this change requires a catversion bump.
>
> > +INSERT INTO test_chunk_id(v1,v2)
> > + VALUES (repeat('x', 1), repeat('x', 2048));
>
> Is this guaranteed to be TOASTed for all possible page sizes?
Should we use block_size?
SHOW block_size \gset
INSERT INTO test_chunk_id(v1,v2)
VALUES (repeat('x', 1), repeat('x', (:block_size / 4)));
I think this will work in various page sizes.
I've attached a patch in which the test is updated.
Regards,
Yugo Nagata
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From c3b99418d1ae3a8235db9bedd95e7d6ac1556f90 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v8] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 27 +
src/test/regress/sql/misc_functions.sql | 24
5 files changed, 112 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 291ed876fc..443ca854a6 100644
--- a/src/include/catalog/pg_
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 7 Mar 2024 16:56:17 -0600
Nathan Bossart wrote:
> On Mon, Feb 05, 2024 at 04:28:23PM +0900, Yugo NAGATA wrote:
> > On Thu, 1 Feb 2024 17:59:56 +0800
> > jian he wrote:
> >> v6 patch looks good.
> >
> > Thank you for your review and updating the status to RwC!
>
> I think this one needs a (pretty trivial) rebase. I spent a few minutes
> testing it out and looking at the code, and it seems generally reasonable
Thank you for your review.
I've attached a rebased patch.
> to me. Do you think it's worth adding something like a
> pg_column_toast_num_chunks() function that returns the number of chunks for
> the TOASTed value, too?
If we want to know the number of chunks of a specified chunk_id,
we can get this by the following query.
postgres=# SELECT id, (SELECT count(*) FROM pg_toast.pg_toast_16384 WHERE
chunk_id = id)
FROM (SELECT pg_column_toast_chunk_id(v) AS id FROM t);
id | count
---+---
16389 | 3
16390 | 287
(2 rows)
However, if there are needs for getting such information in a
simpler way, it might be worth making a new function.
Regards,
Yugo Nagata
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From 7cd0a1cbeae11dea4f4020c71cd0a4550e17b26b Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v7] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 291ed876fc..443ca854a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7447,6 +7447,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value',
+ proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype => 'oid',
+ proargtypes =&g
Fix cancellation check in ExecQueryAndProcessResults
Hi,
While looking ExecQueryAndProcessResults, I found the following code.
/*
* If SIGINT is sent while the query is processing, the interrupt will be
* consumed. The user's intention, though, is to cancel the entire watch
* process, so detect a sent cancellation request and exit in this case.
*/
if (is_watch && cancel_pressed)
{
ClearOrSaveAllResults();
return 0;
}
I guess the intention is that when the query is cancelled during \watch process,
we want to exit early before handling the error. However, we cannot detect the
cancel at this timing because currently we use PQsendQuery which is asynchronous
and does not wait the result. We have to check cancel_pressed after PQgetResult
call. I'm also attached a patch for this, with some comments fix.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 76e01b02a3..7b83e221fd 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1406,9 +1406,9 @@ DescribeQuery(const char *query, double *elapsed_msec)
* For other commands, the results are processed normally, depending on their
* status.
*
- * Returns 1 on complete success, 0 on interrupt and -1 or errors. Possible
- * failure modes include purely client-side problems; check the transaction
- * status for the server-side opinion.
+ * Returns 1 on complete success, 0 on interrupt or results less than min_rows
+ * rows, and -1 on errors. Possible failure modes include purely client-side
+ * problems; check the transaction status for the server-side opinion.
*
* Note that on a combined query, failure does not mean that nothing was
* committed.
@@ -1450,6 +1450,9 @@ ExecQueryAndProcessResults(const char *query,
return -1;
}
+ /* first result */
+ result = PQgetResult(pset.db);
+
/*
* If SIGINT is sent while the query is processing, the interrupt will be
* consumed. The user's intention, though, is to cancel the entire watch
@@ -1461,8 +1464,6 @@ ExecQueryAndProcessResults(const char *query,
return 0;
}
- /* first result */
- result = PQgetResult(pset.db);
if (min_rows > 0 && PQntuples(result) < min_rows)
{
return_early = true;
Re: Remove unnecessary code from psql's watch command
On Wed, 06 Mar 2024 13:03:39 -0500
Tom Lane wrote:
> Michael Paquier writes:
> > On Tue, Mar 05, 2024 at 10:05:52PM +0900, Yugo NAGATA wrote:
> >> In the current code of do_watch(), sigsetjmp is called if WIN32
> >> is defined, but siglongjmp is not called in the signal handler
> >> in this condition. On Windows, currently, cancellation is checked
> >> only by cancel_pressed, and calling sigsetjmp in do_watch() is
> >> unnecessary. Therefore, we can remove code around sigsetjmp in
> >> do_watch(). I've attached the patch for this fix.
>
> > Re-reading the top comment of sigint_interrupt_enabled, it looks like
> > you're right here. As long as we check for cancel_pressed there
> > should be no need for any special cancellation handling here.
>
> I don't have Windows here to test on, but does the WIN32 code
> path work at all? It looks to me like cancel_pressed becoming
> true doesn't get us to exit the outer loop, only the inner delay
> one, meaning that trying to control-C out of a \watch will just
> cause it to repeat the command even faster. That path isn't
> setting the "done" variable, and it isn't checking it either,
> because all of that only happens in the other #ifdef arm.
The outer loop is eventually exited even because PSQLexecWatch returns 0
when cancel_pressed = 0. However, it happens after executing an extra
query in this function not just after exit of the inner loop. Therefore,
it would be better to adding set and check of "done" in WIN32, too.
I've attached the updated patch
(v2_remove_unnecessary_code_in_psql_watch.patch).
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..6827d8b8d0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,30 +5312,22 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
pg_usleep(s * 1000L);
if (cancel_pressed)
+ {
+done = true;
break;
+ }
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, , NULL);
@@ -5369,9 +5361,9 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
/* Unblock SIGINT so that slow queries can be interrupted. */
sigprocmask(SIG_UNBLOCK, , NULL);
+#endif
if (done)
break;
-#endif
}
if (pagerpipe)
Remove unnecessary code from psql's watch command
Hi,
In the current code of do_watch(), sigsetjmp is called if WIN32
is defined, but siglongjmp is not called in the signal handler
in this condition. On Windows, currently, cancellation is checked
only by cancel_pressed, and calling sigsetjmp in do_watch() is
unnecessary. Therefore, we can remove code around sigsetjmp in
do_watch(). I've attached the patch for this fix.
Regards,
Yugo Ngata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..c03e47744e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,20 +5312,10 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
@@ -5335,7 +5325,6 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
break;
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, , NULL);
Re: Incremental View Maintenance, take 2
On Mon, 4 Sep 2023 16:48:02 +0800
jian he wrote:
> other ideas based on v29.
>
> src/include/utils/rel.h
> 680: #define RelationIsIVM(relation) ((relation)->rd_rel->relisivm)
> I guess it would be better to add some comments to address the usage.
> Since all peer macros all have some comments.
OK. I will add comments on this macro.
> pg_class change, I guess we need bump CATALOG_VERSION_NO?
CATALOG_VERSION_NO is frequently bumped up when new features are
committed, so including it in the patch causes frequent needs for
rebase during the review of the patch even if no meaningful change
is made. Therefore, I wonder we don't have to included it in the
patch at this time.
> small issue. makeIvmAggColumn and calc_delta need to add an empty
> return statement?
I'm sorry but I could not understand what you suggested, so could
you give me more explanation?
> style issue. in gram.y, "incremental" upper case?
> + CREATE OptNoLog incremental MATERIALIZED VIEW
> create_mv_target AS SelectStmt opt_with_data
This "incremental" is defined as INCREMENTAL or empty, as below.
incremental:INCREMENTAL { $$ = true; }
| /*EMPTY*/ { $$ = false; }
> I don't know how pgident works, do you need to add some keywords to
> src/tools/pgindent/typedefs.list to make indentation work?
I'm not sure typedefs.list should be updated in each patch, because
tools/pgindent/README said that the latest typedef file is downloaded
from the buildfarm when pgindent is run.
> in
> /* If this is not the last AFTER trigger call, immediately exit. */
> Assert (entry->before_trig_count >= entry->after_trig_count);
> if (entry->before_trig_count != entry->after_trig_count)
> return PointerGetDatum(NULL);
>
> before returning NULL, do you also need clean_up_IVM_hash_entry? (I
> don't know when this case will happen)
No, clean_up_IVM_hash_entry is not necessary in this case.
When multiple tables are updated in a statement, statement-level AFTER
triggers collects every information of the tables, and the last AFTER
trigger have to perform the actual maintenance of the view. To make sure
this, the number that BEFORE and AFTER trigger is fired is counted
respectively, and when they match it is regarded the last AFTER trigger
call performing the maintenance. Until this, collected information have
to keep, so we cannot call clean_up_IVM_hash_entry.
> in
> /* Replace the modified table with the new delta table and
> calculate the new view delta*/
> replace_rte_with_delta(rte, table, true, queryEnv);
> refresh_matview_datafill(dest_new, query, queryEnv, tupdesc_new, "");
>
> replace_rte_with_delta does not change the argument: table, argument:
> queryEnv. refresh_matview_datafill just uses the partial argument of
> the function calc_delta. So I guess, I am confused by the usage of
> replace_rte_with_delta. also I think it should return void, since you
> just modify the input argument. Here refresh_matview_datafill is just
> persisting new delta content to dest_new?
Yes, refresh_matview_datafill executes the query and the result rows to
"dest_new". And, replace_rte_with_delta updates the input argument "rte"
and returns the result to it, so it may be better that this returns void,
as you suggested.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Fri, 1 Sep 2023 15:42:17 +0800
jian he wrote:
I apologize for this late reply.
> I added a new function append_update_set_caluse, and deleted
> functions: {append_set_clause_for_count, append_set_clause_for_sum,
> append_set_clause_for_avg, append_set_clause_for_minmax}
>
> I guess this way is more extensible/generic than yours.
Do you mean that consolidating such functions to a general function
make easier to support a new aggregate function in future? I'm not
convinced completely yet it because your suggestion seems that every
functions' logic are just put into a new function, but providing a
common interface might make a sense a bit.
By the way, when you attach files other than updated patches that
can be applied to master branch, using ".patch" or ".diff" as the
file extension help to avoid to confuse cfbot (for example, like
basedon_v29_matview_c_refactor_update_set_clause.patch.txt).
> src/backend/commands/matview.c
> 2268: /* For tuple deletion */
> maybe "/* For tuple deletion and update*/" is more accurate?
This "deletion" means deletion of tuple from the view rather
than DELETE statement, so I think this is ok.
> Since the apply delta query is quite complex, I feel like adding some
> "if debug then print out the final querybuf.data end if" would be a
> good idea.
Agreed, it would be helpful for debugging. I think it would be good
to add a debug macro that works if DEBUG_IVM is defined rather than
adding GUC like debug_print_..., how about it?
> we add hidden columns somewhere, also to avoid corner cases, so maybe
> somewhere we should assert total attribute number is sane.
The number of hidden columns to be added depends on the view definition
query, so I wonder the Assert condition would be a bit complex. Could
you explain what are you assume about like for example?
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Small fix on query_id_enabled
On Wed, 14 Feb 2024 07:21:54 +0900 Michael Paquier wrote: > On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote: > > +1! > > Okay, applied as-is, then. Thank you! Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA
Re: Small fix on query_id_enabled
On Sat, 10 Feb 2024 10:19:15 +0900 Michael Paquier wrote: > On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote: > > On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote: > >> Also, I think the name is a bit confusing for the same reason, that is, > >> query_id_enabled may be false even when query id is computed in fact. > >> > >> Actually, this does not matter because we use IsQueryIdEnabled to check > >> if query id is enabled, instead of referring to a global variable > >> (query_id_enabled or compute_query_id). But, just for making a code a bit > >> more readable, how about renaming this to query_id_required which seems to > >> stand for the meaning more correctly? > > > > -1 for renaming to avoid breaking extensions that might access it. We > > should > > simply document for compute_query_id and query_id_enabled declaration that > > one > > should instead use IsQueryIdEnabled() if they're interested in whether the > > core > > queryid are computed or not. > > Agreed. A renaming would involve more pain than gain. Improving the > comments around how to all that would be good enough, my 2c. Thank you both for your comments. I agreed with not renaming it. I attached a updated patch that adds comments noting to use IsQueryIdEnabled() instead of accessing the variables directly. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c index e489bfceb5..82f725baaa 100644 --- a/src/backend/nodes/queryjumblefuncs.c +++ b/src/backend/nodes/queryjumblefuncs.c @@ -42,7 +42,13 @@ /* GUC parameters */ int compute_query_id = COMPUTE_QUERY_ID_AUTO; -/* True when compute_query_id is ON, or AUTO and a module requests them */ +/* + * True when compute_query_id is ON or AUTO, and a module requests them. + * + * Note that IsQueryIdEnabled() should be used instead of checking + * query_id_enabled or compute_query_id directly when we want to know + * whether query identifiers are computed in the core or not. + */ bool query_id_enabled = false; static void AppendJumble(JumbleState *jstate,
Small fix on query_id_enabled
Hi,
I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON.
/* True when compute_query_id is ON, or AUTO and a module requests them */
bool query_id_enabled = false;
Should we fix this as following (just fixing the place of a comma) ?
/* True when compute_query_id is ON or AUTO, and a module requests them */
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.
Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?
I attached a patch for above fixes.
Although renaming might not be acceptable since changing global variables
may affect third party tools, I think the comment should be fixed at least.
IMHO, it seems better to make this variable static not to be accessed directly.
However, I left it as is because this is used in a static inline function.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb5..e4fbcf0d9f 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -42,8 +42,8 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
-/* True when compute_query_id is ON, or AUTO and a module requests them */
-bool query_id_enabled = false;
+/* True when compute_query_id is ON or AUTO, and a module requests them */
+bool query_id_required = false;
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
@@ -145,7 +145,7 @@ void
EnableQueryId(void)
{
if (compute_query_id != COMPUTE_QUERY_ID_OFF)
- query_id_enabled = true;
+ query_id_required = true;
}
/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a066800a1c..4bcaf6d71c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -532,7 +532,7 @@ typedef struct
pg_time_t first_syslogger_file_time;
bool redirection_done;
bool IsBinaryUpgrade;
- bool query_id_enabled;
+ bool query_id_required;
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
@@ -6103,7 +6103,7 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
- param->query_id_enabled = query_id_enabled;
+ param->query_id_required = query_id_required;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -6348,7 +6348,7 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
- query_id_enabled = param->query_id_enabled;
+ query_id_required = param->query_id_required;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index f1c55c8067..3b2e1f8018 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -67,7 +67,7 @@ extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query);
extern void EnableQueryId(void);
-extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_required;
/*
* Returns whether query identifier computation has been enabled, either
@@ -80,7 +80,7 @@ IsQueryIdEnabled(void)
return false;
if (compute_query_id == COMPUTE_QUERY_ID_ON)
return true;
- return query_id_enabled;
+ return query_id_required;
}
#endif /* QUERYJUMBLE_H */
Re: Rename setup_cancel_handler in pg_dump
On Wed, 7 Feb 2024 22:59:48 +0100 Daniel Gustafsson wrote: > > On 1 Feb 2024, at 02:21, Yugo NAGATA wrote: > > On Tue, 30 Jan 2024 13:44:28 +0100 > > Daniel Gustafsson wrote: > > >> -setup_cancel_handler(void) > >> +pg_dump_setup_cancel_handler(void) > >> > >> We don't have any other functions prefixed with pg_dump_, based on the > >> naming > >> of the surrounding code in the file I wonder if set_cancel_handler is a > >> more > >> appropriate name? > > > > Agreed. Here is a updated patch. > > Sleeping on it I still think this is a good idea, and hearing no objections I > went ahead with this. Thanks for the patch! Thank you! Regards, Yugo Nagata > > -- > Daniel Gustafsson > -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
On Wed, 24 Jan 2024 22:17:44 +0900 Yugo NAGATA wrote: > Attached is the updated patch, v6. Currently, on non-Windows, SIGINT is received only by thread #0. CancelRequested is checked during the loop in the thread, and queries are cancelled if it is set. However, once thread #0 exits the loop due to some runtime error and starts waiting in pthread_join, there is no opportunity to cancel queries run by other threads. In addition, if -C option is specified, connections are created for each transaction, so cancel objects (PGcancel) also have to be recreated at each time in each thread. However, these cancel objects are used in a specific thread to perform cancel for all queries, which is not safe because a thread refers to objects updated by other threads. I think the first problem would be addressed by any of followings. (1a) Perform cancels in the signal handler. The signal handler will be called even while the thread 0 is blocked in pthread_join. This is safe because PQcancel is callable from a signal handler. (1b) Create an additional dedicated thread that calls sigwait on SIGINT and performs query cancel. As far as I studied, creating such dedicated thread calling sigwait is a typical way to handle signal in multi-threaded programming. (1c) Each thread performs cancel for queries run by each own, rather than that thread 0 cancels all queries. For the purpose, pthread_kill might be used to interrupt threads blocked in wait_on_socket_set. The second one would be addressed by any of followings. (2a) Use critical section when accessing PGcancel( e.g by using pthread_mutex (non-Windows) or EnterCriticalSection (Windows)). On non-Windows, we cannot use this way when calling PQcancel in a signal handler ((1a) above) because acquiring a lock is not re-entrant. (2b) Perform query cancel in each thread that has created the connection (same as (1c) above). Considering both, possible combination would be either (1b)&(2a) or (1c)&(2b). I would prefer the former way, because creating the dedicated thread handling SIGINT signal and canceling all queries seems simpler and safer than calling pthread_kill in the SIGINT signal handler to send another signal to other threads. I'll update the patch in this way soon. Regards, Yugo Nagata > > > Best reagards, > > -- > > Tatsuo Ishii > > SRA OSS LLC > > English: http://www.sraoss.co.jp/index_en/ > > Japanese:http://www.sraoss.co.jp > > > > > > > -- > Yugo NAGATA -- Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Mon, 5 Feb 2024 14:26:46 +0800
jian he wrote:
> On Mon, Feb 5, 2024 at 10:29 AM torikoshia wrote:
> >
> > Hi,
> >
> > On 2024-02-03 15:22, jian he wrote:
> > > The idea of on_error is to tolerate errors, I think.
> > > if a column has a not null constraint, let it cannot be used with
> > > (on_error 'null')
> >
> > > + /*
> > > +* we can specify on_error 'null', but it can only apply to
> > > columns
> > > +* don't have not null constraint.
> > > + */
> > > + if (att->attnotnull && cstate->opts.on_error ==
> > > COPY_ON_ERROR_NULL)
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> > > +errmsg("copy on_error 'null' cannot be used with
> > > not null constraint column")));
> >
> > This means we cannot use ON_ERROR 'null' even when there is one column
> > which have NOT NULL constraint, i.e. primary key, right?
> > IMHO this is strong constraint and will decrease the opportunity to use
> > this feature.
>
> I don't want to fail in the middle of bulk inserts,
> so I thought immediately erroring out would be a great idea.
> Let's see what other people think.
I also think this restriction is too strong because it is very
common that a table has a primary key, unless there is some way
to specify columns that can be set to NULL. Even when ON_ERROR
is specified, any constraint violation errors cannot be generally
ignored, so we cannot elimiate the posibility COPY FROM fails in
the middle due to invalid data, anyway.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Tue, 06 Feb 2024 09:39:09 +0900 (JST)
Kyotaro Horiguchi wrote:
> At Mon, 5 Feb 2024 17:22:56 +0900, Yugo NAGATA wrote in
> > On Mon, 05 Feb 2024 11:28:59 +0900
> > torikoshia wrote:
> >
> > > > Based on this, I've made a patch.
> > > > based on COPY Synopsis: ON_ERROR 'error_action'
> > > > on_error 'null', the keyword NULL should be single quoted.
> > >
> > > As you mentioned, single quotation seems a little odd..
> > >
> > > I'm not sure what is the best name and syntax for this feature, but
> > > since current error_action are verbs('stop' and 'ignore'), I feel 'null'
> > > might not be appropriate.
> >
> > I am not in favour of using 'null' either, so I suggested to use
> > "set_to_null" or more generic syntax like "set_to (col, val)" in my
> > previous post[1], although I'm not convinced what is the best either.
> >
> > [1]
> > https://www.postgresql.org/message-id/20240129172858.ccb6c77c3be95a295e2b2b44%40sraoss.co.jp
>
> Tom sugggested using a separate option, and I agree with the
> suggestion. Taking this into consideration, I imagined something like
> the following, for example. Although I'm not sure we are actually
> going to do whole-tuple replacement, the action name in this example
> has the suffix '-column'.
>
> COPY (on_error 'replace-colomn', replacement 'null') ..
Thank you for your information. I've found a post[1] you mentioned,
where adding a separate option for error log destination was suggested.
Considering consistency with other options, adding a separate option
would be better if we want to specify a value to replace the invalid
value, without introducing a complex syntax that allows options with
more than one parameters. Maybe, if we allow to use values for the
replacement other than NULL, we have to also add a option to specify
a column (or a type) for each replacement value. Or, we may add a
option to specify a list of replacement values as many as the number of
columns, each of whose default is NULL.
Anyway, I prefer 'replace" (or 'set_to') to just 'null' as the option
value.
[1] https://www.postgresql.org/message-id/2070915.1705527477%40sss.pgh.pa.us
Regards,
Yugo Nagata
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
--
Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Mon, 05 Feb 2024 11:28:59 +0900
torikoshia wrote:
> > Based on this, I've made a patch.
> > based on COPY Synopsis: ON_ERROR 'error_action'
> > on_error 'null', the keyword NULL should be single quoted.
>
> As you mentioned, single quotation seems a little odd..
>
> I'm not sure what is the best name and syntax for this feature, but
> since current error_action are verbs('stop' and 'ignore'), I feel 'null'
> might not be appropriate.
I am not in favour of using 'null' either, so I suggested to use
"set_to_null" or more generic syntax like "set_to (col, val)" in my
previous post[1], although I'm not convinced what is the best either.
[1]
https://www.postgresql.org/message-id/20240129172858.ccb6c77c3be95a295e2b2b44%40sraoss.co.jp
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 1 Feb 2024 17:59:56 +0800 jian he wrote: > On Thu, Feb 1, 2024 at 12:45 PM Yugo NAGATA wrote: > > > > Here is a updated patch, v6. > > v6 patch looks good. Thank you for your review and updating the status to RwC! Regards, Yugo Nagata -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Sat, 3 Feb 2024 01:51:56 +0200 Alexander Korotkov wrote: > On Fri, Feb 2, 2024 at 10:07 PM David G. Johnston > wrote: > > On Thu, Feb 1, 2024 at 11:59 PM Yugo NAGATA wrote: > >> > >> > >> I attached a updated patch including fixes you pointed out above. > >> > > > > Removed "which"; changed "occupying" to "occupy" > > Removed on of the two "amounts" > > Changed "unacceptable to the input function" to just "converting" as that > > is what the average reader is more likely to be thinking. > > The rest was good. > > Thanks to everybody in the thread. > Pushed. Thanks! > > -- > Regards, > Alexander Korotkov -- Yugo NAGATA
Re: InstallXLogFileSegment() vs concurrent WAL flush
On Fri, 2 Feb 2024 11:18:18 +0100
Thomas Munro wrote:
> Hi,
>
> New WAL space is created by renaming a file into place. Either a
> newly created file with a temporary name or, ideally, a recyclable old
> file with a name derived from an old LSN. I think there is a data
> loss window between rename() and fsync(parent_directory). A
> concurrent backend might open(new_name), write(), fdatasync(), and
> then we might lose power before the rename hits the disk. The data
> itself would survive the crash, but recovery wouldn't be able to find
> and replay it. That might break the log-before-data rule or forget a
> transaction that has been reported as committed to a client.
>
> Actual breakage would presumably require really bad luck, and I
> haven't seen this happen or anything, it just occurred to me while
> reading code, and I can't see any existing defences.
>
> One simple way to address that would be to make XLogFileInitInternal()
> wait for InstallXLogFileSegment() to finish. It's a little
Or, can we make sure the rename is durable by calling fsync before
returning the fd, as a patch attached here?
Regards,
Yugo Nagata
> pessimistic to do that unconditionally, though, as then you have to
> wait even for rename operations for segment files later than the one
> you're opening, so I thought about how to skip waiting in that case --
> see 0002. I'm not sure if it's worth worrying about or not.
--
Yugo NAGATA
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..862109ca3b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3062,7 +3062,11 @@ XLogFileInitInternal(XLogSegNo logsegno, TimeLineID logtli,
errmsg("could not open file \"%s\": %m", path)));
}
else
+ {
+ fsync_fname_ext(path, false, false, ERROR);
+ fsync_parent_path(path, ERROR);
return fd;
+ }
/*
* Initialize an empty (all zeroes) segment. NOTE: it is possible that
Re: Checking MINIMUM_VERSION_FOR_WAL_SUMMARIES
On Fri, 2 Feb 2024 01:11:27 +0100
Artur Zakirov wrote:
> Hi hackers,
>
> during reading the source code of new incremental backup functionality
> I noticed that the following condition can by unintentional:
>
> /*
> * For newer server versions, likewise create pg_wal/summaries
> */
> if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
> {
> ...
>
> if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
> errno != EEXIST)
> pg_fatal("could not create directory \"%s\": %m", summarydir);
> }
>
> This is from src/bin/pg_basebackup/pg_basebackup.c.
>
> Is the condition correct? Shouldn't it be ">=". Otherwise the function
> will create "/summaries" only for older PostgreSQL versions.
>
> I've attached a patch to fix it in case this is a typo.
I also think it should be ">="
Also, if so, I don't think the check of MINIMUM_VERSION_FOR_PG_WAL
in the block is required, because the directory name is always pg_wal
in the new versions.
Regards,
Yugo Nagata
>
> --
> Artur
--
Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 02 Feb 2024 11:29:41 +0900 torikoshia wrote: > On 2024-02-01 15:16, Yugo NAGATA wrote: > > On Mon, 29 Jan 2024 15:47:25 +0900 > > Yugo NAGATA wrote: > > > >> On Sun, 28 Jan 2024 19:14:58 -0700 > >> "David G. Johnston" wrote: > >> > >> > > Also, I think "invalid input syntax" is a bit ambiguous. For example, > >> > > COPY FROM raises an error when the number of input column does not > >> > > match > >> > > to the table schema, but this error is not ignored by ON_ERROR while > >> > > this seems to fall into the category of "invalid input syntax". > >> > > >> > > >> > > >> > It is literally the error text that appears if one were not to ignore it. > >> > It isn’t a category of errors. But I’m open to ideas here. But being > >> > explicit with what on actually sees in the system seemed preferable to > >> > inventing new classification terms not otherwise used. > >> > >> Thank you for explanation! I understood the words was from the error > >> messages > >> that users actually see. However, as Torikoshi-san said in [1], errors > >> other > >> than valid input syntax (e.g. range error) can be also ignored, > >> therefore it > >> would be better to describe to be ignored errors more specifically. > >> > >> [1] > >> https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com > >> > >> > > >> > > > >> > > So, keeping consistency with the existing description, we can say: > >> > > > >> > > "Specifies which how to behave when encountering an error due to > >> > > column values unacceptable to the input function of each attribute's > >> > > data type." > >> > > >> > > >> > Yeah, I was considering something along those lines as an option as well. > >> > But I’d rather add that wording to the glossary. > >> > >> Although I am still be not convinced if we have to introduce the words > >> "soft error" to the documentation, I don't care it if there are no > >> other > >> opposite opinions. > > > > Attached is a updated patch v3, which is a version that uses the above > > wording instead of "soft error". > > > >> > > >> > > Currently, ON_ERROR doesn't support other soft errors, so it can > >> > > explain > >> > > it more simply without introducing the new concept, "soft error" to > >> > > users. > >> > > > >> > > > >> > Good point. Seems we should define what user-facing errors are ignored > >> > anywhere in the system and if we aren’t consistently leveraging these in > >> > all areas/commands make the necessary qualifications in those specific > >> > places. > >> > > >> > >> > > I think "left in a deleted state" is also unclear for users because > >> > > this > >> > > explains the internal state but not how looks from user's view.How > >> > > about > >> > > leaving the explanation "These rows will not be visible or accessible" > >> > > in > >> > > the existing statement? > >> > > > >> > > >> > Just visible then, I don’t like an “or” there and as tuples at least they > >> > are accessible to the system, in vacuum especially. But I expected the > >> > user to understand “as if you deleted it” as their operational concept > >> > more > >> > readily than visible. I think this will be read by people who haven’t > >> > read > >> > MVCC to fully understand what visible means but know enough to run vacuum > >> > to clean up updated and deleted data as a rule. > >> > >> Ok, I agree we can omit "or accessible". How do you like the > >> followings? > >> Still redundant? > >> > >> "If the command fails, these rows are left in a deleted state; > >> these rows will not be visible, but they still occupy disk space. " > > > > Also, the above statement is used in the patch. > > Thanks for updating the patch! > > I like your description which doesn't use the word soft error. Thank you for your comments! > > Here are minor comments: > >
Re: Small fix on COPY ON_ERROR document
On Mon, 29 Jan 2024 15:47:25 +0900
Yugo NAGATA wrote:
> On Sun, 28 Jan 2024 19:14:58 -0700
> "David G. Johnston" wrote:
>
> > > Also, I think "invalid input syntax" is a bit ambiguous. For example,
> > > COPY FROM raises an error when the number of input column does not match
> > > to the table schema, but this error is not ignored by ON_ERROR while
> > > this seems to fall into the category of "invalid input syntax".
> >
> >
> >
> > It is literally the error text that appears if one were not to ignore it.
> > It isn’t a category of errors. But I’m open to ideas here. But being
> > explicit with what on actually sees in the system seemed preferable to
> > inventing new classification terms not otherwise used.
>
> Thank you for explanation! I understood the words was from the error messages
> that users actually see. However, as Torikoshi-san said in [1], errors other
> than valid input syntax (e.g. range error) can be also ignored, therefore it
> would be better to describe to be ignored errors more specifically.
>
> [1]
> https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com
>
> >
> > >
> > > So, keeping consistency with the existing description, we can say:
> > >
> > > "Specifies which how to behave when encountering an error due to
> > > column values unacceptable to the input function of each attribute's
> > > data type."
> >
> >
> > Yeah, I was considering something along those lines as an option as well.
> > But I’d rather add that wording to the glossary.
>
> Although I am still be not convinced if we have to introduce the words
> "soft error" to the documentation, I don't care it if there are no other
> opposite opinions.
Attached is a updated patch v3, which is a version that uses the above
wording instead of "soft error".
> >
> > > Currently, ON_ERROR doesn't support other soft errors, so it can explain
> > > it more simply without introducing the new concept, "soft error" to users.
> > >
> > >
> > Good point. Seems we should define what user-facing errors are ignored
> > anywhere in the system and if we aren’t consistently leveraging these in
> > all areas/commands make the necessary qualifications in those specific
> > places.
> >
>
> > > I think "left in a deleted state" is also unclear for users because this
> > > explains the internal state but not how looks from user's view.How about
> > > leaving the explanation "These rows will not be visible or accessible" in
> > > the existing statement?
> > >
> >
> > Just visible then, I don’t like an “or” there and as tuples at least they
> > are accessible to the system, in vacuum especially. But I expected the
> > user to understand “as if you deleted it” as their operational concept more
> > readily than visible. I think this will be read by people who haven’t read
> > MVCC to fully understand what visible means but know enough to run vacuum
> > to clean up updated and deleted data as a rule.
>
> Ok, I agree we can omit "or accessible". How do you like the followings?
> Still redundant?
>
> "If the command fails, these rows are left in a deleted state;
> these rows will not be visible, but they still occupy disk space. "
Also, the above statement is used in the patch.
Regards,
Yugo Nagata
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 21a5c4a052..10cfc3f0ad 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -90,6 +90,13 @@ COPY { table_name [ ( pg_stat_progress_copy view. See
for details.
+
+
+By default, COPY will fail if it encounters an error
+during processing. For use cases where a best-effort attempt at loading
+the entire file is desired, the ON_ERROR clause can
+be used to specify some other behavior.
+
@@ -378,17 +385,20 @@ COPY { table_name [ ( ON_ERROR
- Specifies which
- error_action to perform when there is malformed data in the input.
- Currently, only stop (default) and ignore
- values are supported.
- If the stop value is specified,
- COPY stops operation at the first error.
- If the ignore value is specified,
- COPY skips malformed data and continues copying data.
- The option is allowed only in COPY FROM.
- Only stop value is allowed when
- using binary format.
+ Specifies which how to behave when encountering
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 14:57:20 +0800
jian he wrote:
> On Tue, Jan 30, 2024 at 1:56 PM Yugo NAGATA wrote:
> >
> > I attached the correct one, v4.
> >
>
> +-- Test pg_column_toast_chunk_id:
> +-- Check if the returned chunk_id exists in the TOAST table
> +CREATE TABLE test_chunk_id (v1 text, v2 text);
> +INSERT INTO test_chunk_id VALUES (
> + repeat('0123456789', 10), -- v1: small enough not to be TOASTed
> + repeat('0123456789', 10)); -- v2: large enough to be TOASTed
>
> select pg_size_pretty(10::bigint);
> return 98kb.
>
> I think this is just too much, maybe I didn't consider the
> implications of compression.
> Anyway, I refactored the tests, making the toast value size be small.
Actually the data is compressed and the size is much smaller,
but I agree with you it is better not to generate large data unnecessarily.
I rewrote the test to disallow compression in the toast data using
"ALTER TABLE ... SET STORAGE EXTERNAL". In this case, any text larger
than 2k will be TOASTed on disk without compression, and it makes the
test simple, not required to use string_agg.
>
> I aslo refactor the doc.
> pg_column_toast_chunk_id entry will be right after pg_column_compression
> entry.
> You can check the screenshot.
I found the document order was not changed between my patch and yours.
In both, pg_column_toast_chunk_id entry is right after
pg_column_compression.
Here is a updated patch, v6.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From dfec0fdb52c6aad8846b5c984e111dc8b2985e1a Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v6] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6788ba8ef4..4116aaff7a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28502,6 +28502,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 29af4ce65d..9ab71646e3 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7454,6 +7454,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile =&g
Re: Rename setup_cancel_handler in pg_dump
On Tue, 30 Jan 2024 13:44:28 +0100
Daniel Gustafsson wrote:
> > On 26 Jan 2024, at 01:42, Yugo NAGATA wrote:
>
> > I am proposing it because there is a public function with
> > the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> > does not include fe_utils/cancel.h, so there is no conflict,
> > but I think it is better to use different names to reduce
> > possible confusion.
>
> Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
> other frontend utils, I can see the risk of confusion.
Thank you for looking into it!
> -setup_cancel_handler(void)
> +pg_dump_setup_cancel_handler(void)
>
> We don't have any other functions prefixed with pg_dump_, based on the naming
> of the surrounding code in the file I wonder if set_cancel_handler is a more
> appropriate name?
Agreed. Here is a updated patch.
Regards,
Yugo Nagata
>
> --
> Daniel Gustafsson
>
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..a09247fae4 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void set_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that set_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ set_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 13:47:45 +0800
jian he wrote:
> On Tue, Jan 30, 2024 at 12:35 PM Yugo NAGATA wrote:
> >
> >
> > Sorry, I also attached a wrong file.
> > Attached is the correct one.
> I think you attached the wrong file again. also please name it as v4.
Opps..sorry, again.
I attached the correct one, v4.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 8c0f9993c49d1d4ed1bb3e10ba26652da98cd41e Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v4] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 18 +
src/test/regress/sql/misc_functions.sql | 18 +
5 files changed, 97 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 210c7c0b02..2d82331323 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 58811a6530..1d4521ac1f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7454,6 +7454,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value',
+ proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype => 'oid',
+ proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
{ oid => '2322',
descr => 'total disk space usage for the specified tablespace',
proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 9302134077..6a1fcc22f5 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -670,3 +670,21 @@ FROM pg_walfile_name_offset('0/0'::pg_lsn + :segment_size - 1),
0 | t
(1 row)
+-- Test pg_column_toast_chunk_id:
+-- Check if the returned chunk_id exists in the TOAST table
+CREATE TABLE test_chunk_id (v1 text, v2 text);
+INSERT INTO test_chunk_id VALUES (
+ repeat('0123456789', 10), -- v1: small enough not to be TOASTed
+ repeat('0123456789', 10)); -- v2: large enough to be
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 12:12:31 +0800
jian he wrote:
> On Fri, Jan 26, 2024 at 8:42 AM Yugo NAGATA wrote:
> >
> > On Tue, 2 Jan 2024 08:00:00 +0800
> > jian he wrote:
> >
> > > On Mon, Nov 6, 2023 at 8:00 AM jian he
> > > wrote:
> > > >
> > > > minor doc issues.
> > > > Returns the chunk id of the TOASTed value, or NULL if the value is not
> > > > TOASTed.
> > > > Should it be "chunk_id"?
> >
> > Thank you for your suggestion. As you pointed out, it is called "chunk_id"
> > in the documentation, so I rewrote it and also added a link to the section
> > where the TOAST table structure is explained.
> >
> > > > you may place it after pg_create_logical_replication_slot entry to
> > > > make it look like alphabetical order.
> >
> > I've been thinking about where we should place the function in the doc,
> > and I decided place it in the table of Database Object Size Functions
> > because I think pg_column_toast_chunk_id also would assist understanding
> > the result of size functions as similar to pg_column_compression; that is,
> > those function can explain why a large value in size could be stored in
> > a column.
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index 210c7c0b02..2d82331323 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn +
> pd.segment_number * ps.setting::int + :offset
>
>
>
> +
> +
> +
> + pg_column_toast_chunk_id
> +
> +pg_column_toast_chunk_id ( "any" )
> +oid
> +
> +
> +Shows the chunk_id of an on-disk
> +TOASTed value. Returns NULL
> +if the value is un-TOASTed or not on-disk.
> +See for details about
> +TOAST.
> +
> +
>
> v3 patch will place it on `Table 9.97. Replication Management Functions`
> I agree with you. it should be placed after pg_column_compression. but
> apply your patch, it will be at
>
>
> > > > There is no test. maybe we can add following to
> > > > src/test/regress/sql/misc.sql
> > > > create table val(t text);
> > > > INSERT into val(t) SELECT string_agg(
> > > > chr((ascii('B') + round(random() * 25)) :: integer),'')
> > > > FROM generate_series(1,2500);
> > > > select pg_column_toast_chunk_id(t) is not null from val;
> > > > drop table val;
> >
> > Thank you for the test proposal. However, if we add a test, I want
> > to check that the chunk_id returned by the function exists in the
> > TOAST table, and that it returns NULL if the values is not TOASTed.
> > For the purpose, I wrote a test using a dynamic SQL since the table
> > name of the TOAST table have to be generated from the main table's OID.
> >
> > > Hi
> > > the main C function (pg_column_toast_chunk_id) I didn't change.
> > > I added tests as mentioned above.
> > > tests put it on src/test/regress/sql/misc.sql, i hope that's fine.
> > > I placed pg_column_toast_chunk_id in "Table 9.99. Database Object
> > > Location Functions" (below Table 9.98. Database Object Size
> > > Functions).
> >
> > I could not find any change in your patch from my previous patch.
> > Maybe, you attached wrong file. I attached a patch updated based
> > on your review, including the documentation fixes and a test.
> > What do you think about this it?
> >
>
> sorry, I had attached the wrong file.
> but your v3 also has no tests, documentation didn't fix.
> maybe you also attached the wrong file too?
>
Sorry, I also attached a wrong file.
Attached is the correct one.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 97af1b2300ecf07a34923da87a9d84e6aa963956 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v3] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17 ++
src/backend/utils/adt/varlena.c | 41 +
src/include/catalog/pg_proc.dat | 3 +++
3 files changed, 61 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
inde
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Fri, 26 Jan 2024 08:08:29 -0700 "David G. Johnston" wrote: > Hi, > > The option choice of "ignore" in the COPY ON_ERROR clause seems overly > generic. There would seem to be two relevant ways to ignore bad column > input data - drop the entire row or just set the column value to null. I > can see us wanting to provide the set to null option and in any case having > the option name be explicit that it ignores the row seems like a good idea. I am not in favour of renaming the option name "ignore", instead we can use another style of name for the option to set the column value to NULL, for example, "set_to_null". (Maybe, we can make a more generic option "set_to (col, val)" that can set the value of column specified by "col" value to the specified value "val" (e.g. 'N/A') on a soft error, although the syntax would be a bit complex...) IMO, it is more simple to define "ignore" as to skip the entire row rather than having variety of "ignore". Once defined it so, the option to set the column value to NULL should not be called "ignore" because values in other columns will be inserted. Regards, Yugo Nagata > > David J. -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Sun, 28 Jan 2024 19:14:58 -0700 "David G. Johnston" wrote: > > Also, I think "invalid input syntax" is a bit ambiguous. For example, > > COPY FROM raises an error when the number of input column does not match > > to the table schema, but this error is not ignored by ON_ERROR while > > this seems to fall into the category of "invalid input syntax". > > > > It is literally the error text that appears if one were not to ignore it. > It isn’t a category of errors. But I’m open to ideas here. But being > explicit with what on actually sees in the system seemed preferable to > inventing new classification terms not otherwise used. Thank you for explanation! I understood the words was from the error messages that users actually see. However, as Torikoshi-san said in [1], errors other than valid input syntax (e.g. range error) can be also ignored, therefore it would be better to describe to be ignored errors more specifically. [1] https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com > > > > > So, keeping consistency with the existing description, we can say: > > > > "Specifies which how to behave when encountering an error due to > > column values unacceptable to the input function of each attribute's > > data type." > > > Yeah, I was considering something along those lines as an option as well. > But I’d rather add that wording to the glossary. Although I am still be not convinced if we have to introduce the words "soft error" to the documentation, I don't care it if there are no other opposite opinions. > > > Currently, ON_ERROR doesn't support other soft errors, so it can explain > > it more simply without introducing the new concept, "soft error" to users. > > > > > Good point. Seems we should define what user-facing errors are ignored > anywhere in the system and if we aren’t consistently leveraging these in > all areas/commands make the necessary qualifications in those specific > places. > > > I think "left in a deleted state" is also unclear for users because this > > explains the internal state but not how looks from user's view.How about > > leaving the explanation "These rows will not be visible or accessible" in > > the existing statement? > > > > Just visible then, I don’t like an “or” there and as tuples at least they > are accessible to the system, in vacuum especially. But I expected the > user to understand “as if you deleted it” as their operational concept more > readily than visible. I think this will be read by people who haven’t read > MVCC to fully understand what visible means but know enough to run vacuum > to clean up updated and deleted data as a rule. Ok, I agree we can omit "or accessible". How do you like the followings? Still redundant? "If the command fails, these rows are left in a deleted state; these rows will not be visible, but they still occupy disk space. " Regards, Yugo Nagata -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 08:04:45 -0700 "David G. Johnston" wrote: > On Fri, Jan 26, 2024 at 2:30 AM Yugo NAGATA wrote: > > > On Fri, 26 Jan 2024 00:00:57 -0700 > > "David G. Johnston" wrote: > > > > > I will need to make this tweak and probably a couple others to my own > > > suggestions in 12 hours or so. > > > > > > > > And here is my v2. > > Notably I choose to introduce the verbiage "soft error" and then define in > the ON_ERROR clause the specific soft error that matters here - "invalid > input syntax". I am not sure we should use "soft error" without any explanation because it seems to me that the meaning of words is unclear for users. This is explained in src/backend/utils/fmgr/README; * Some callers of datatype input functions (and in future perhaps other classes of functions) pass an instance of ErrorSaveContext. This indicates that the caller wishes to handle "soft" errors without a transaction-terminating exception being thrown: instead, the callee should store information about the error cause in the ErrorSaveContext struct and return a dummy result value. However, this is not mentioned in the documentation anywhere. So, it would be hard for users to understand what "soft error" is because they would not read README. Also, I think "invalid input syntax" is a bit ambiguous. For example, COPY FROM raises an error when the number of input column does not match to the table schema, but this error is not ignored by ON_ERROR while this seems to fall into the category of "invalid input syntax". I found the description as followings in the documentation in COPY: The column values themselves are strings (...) acceptable to the input function, of each attribute's data type. So, keeping consistency with the existing description, we can say: "Specifies which how to behave when encountering an error due to column values unacceptable to the input function of each attribute's data type." Currently, ON_ERROR doesn't support other soft errors, so it can explain it more simply without introducing the new concept, "soft error" to users. > I also note the log message behavior when ignore mode is chosen. I haven't > confirmed that it is accurate but that is readily tweaked if approved of. > + An INFO level context message containing the ignored row count is + emitted at the end of the COPY FROM if at least one row was discarded. The log level is NOTICE not INFO. -COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY -TO, but the target table will already have received -earlier rows in a COPY FROM. These rows will not -be visible or accessible, but they still occupy disk space. This might +The COPY FROM command physically inserts input rows +into the table as it progresses. If the command fails these rows are +left in a deleted state, still occupying disk space. This might amount to a considerable amount of wasted disk space if the failure -happened well into a large copy operation. You might wish to invoke -VACUUM to recover the wasted space. +happened well into a large copy operation. VACUUM +should be used to recover the wasted space. I think "left in a deleted state" is also unclear for users because this explains the internal state but not how looks from user's view.How about leaving the explanation "These rows will not be visible or accessible" in the existing statement? Regards, Yugo Nagata -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 00:00:57 -0700 "David G. Johnston" wrote: > On Thursday, January 25, 2024, Yugo NAGATA wrote: > > > > > Maybe, we can separate the sentese to two, for example: > > > > COPY stops operation at the first error. (The exception is if the error > > is due to data type incompatibility and a value other than stop is > > specified > > to the ON_ERROR option.) > > > > I’d lean more toward saying: > > Copy from can be instructed to ignore errors that arise from casting input > data to the data types of the target columns by setting the on_error option > to ignore. Skipping the entire input row in the process. > > The last part is because another way to ignore would be to set null values > for those columns. That makes sense. Is is a bit ambiguous to just say "skips malformed data"; it might be unclear for users if the data in the problematic column is skipped (NULL is set) or the entire row is skipped. Also, setting null values for those columns could be a future feature of ON_ERROR option. > > That a command stops on an error is the assumed behavior throughout the > system, writing “copy stops operation at the first error.” just seems > really unnecessary. I think we need to describe this default behavior explicitly somewhere, as you suggested in the other post [1]. [1] https://www.postgresql.org/message-id/CAKFQuwZJZ6uaS2B7qpL2FJzWBsnDdzgtbsW3pH9zuT6vC3fH%2Bg%40mail.gmail.com Regards, Yugo Nagata > I will need to make this tweak and probably a couple others to my own > suggestions in 12 hours or so. > > David J. -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Thu, 25 Jan 2024 23:33:22 -0700
"David G. Johnston" wrote:
> On Thu, Jan 25, 2024 at 10:40 PM Yugo NAGATA wrote:
>
> > On Fri, 26 Jan 2024 13:59:09 +0900
> > Masahiko Sawada wrote:
> >
> > > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA
> > wrote:
> > > >
> > > > Hi,
> > > >
> > > > I found that the documentation of COPY ON_ERROR said
> > > > COPY stops operation at the first error when
> > > > "ON_ERROR is not specified.", but it also stop when
> > > > ON_ERROR is specified to the default value, "stop".
> > > >
> >
> >
> I'm finding the current wording surrounding ON_ERROR to not fit in with the
> rest of the page. I've tried to improve things by being a bit more
> invasive.
Introducing the ON_ERROR option changed the behavior of COPY about error
handling to some extent, so it might be good to rewrite a bit more parts
of the documentation as you suggest.
> This first hunk updates the description of the COPY command to include
> describing the purpose of the ON_ERROR clause.
>
> I too am concerned about the word "parsed" here, and "malformed" in the
> more detailed descriptions; this would need to be modified to better
> reflect the circumstances under which ignore happens.
I think "errors due to data type incompatibility" would be nice to describe
the errors to be ignored by ON_ERROR, as used in the NOTICE message output.
> diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
> index 21a5c4a052..5fe4c9f747 100644
> --- a/doc/src/sgml/ref/copy.sgml
> +++ b/doc/src/sgml/ref/copy.sgml
> @@ -90,6 +90,14 @@ COPY { class="parameter">table_name [ ( in the pg_stat_progress_copy view. See
> for details.
>
> +
> +
> +By default, COPY will abort if it encounters errors.
> +For non-binary format COPY FROM the user can specify
> +to instead ignore input rows that cannot be parsed by specifying
> +the ignore option to the ON_ERROR
> +clause.
> +
>
>
>
> The following was, IMO, too much text about something not to worry about,
> COPY TO and ignore mode. The point is dead tuples on error and running
> vacuum. It doesn't really even matter what caused the error, though if the
> error was even before rows started to be imported then obviously there
> would be no dead tuples.
>
> Oh, and the word "first" really isn't adding anything here that we cannot
> reasonably leave to common sense, IMO. We either ignore all errors or stop
> on the first one. There isn't a stop mode that is capable of bypassing
> errors and the ignore mode doesn't have a "first n" option so one assumes
> all errors are ignored.
>
> @@ -576,15 +583,12 @@ COPY class="parameter">count
>
>
>
> -COPY stops operation at the first error when
> -ON_ERROR is not specified. This
> -should not lead to problems in the event of a COPY
> -TO, but the target table will already have received
> -earlier rows in a COPY FROM. These rows will not
> -be visible or accessible, but they still occupy disk space. This might
> -amount to a considerable amount of wasted disk space if the failure
> -happened well into a large copy operation. You might wish to invoke
> -VACUUM to recover the wasted space.
> +A failed COPY FROM command will have performed
> +physical insertion of all rows prior to the malformed one.
As you said that it does not matter what caused the error, I don't think
we have to use "malformed" here. Instead, we could say "we will have
performed physical insertion of rows before the failure."
> +While these rows will not be visible or accessible, they still occupy
> +disk space. This might amount to a considerable amount of wasted disk
> +space if the failure happened well into a large copy operation.
> +VACUUM should be used to recover the wasted space.
>
>
>
>
> David J.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 15:26:55 +0900 Masahiko Sawada wrote: > On Fri, Jan 26, 2024 at 2:40 PM Yugo NAGATA wrote: > > > > On Fri, 26 Jan 2024 13:59:09 +0900 > > Masahiko Sawada wrote: > > > > > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA wrote: > > > > > > > > Hi, > > > > > > > > I found that the documentation of COPY ON_ERROR said > > > > COPY stops operation at the first error when > > > > "ON_ERROR is not specified.", but it also stop when > > > > ON_ERROR is specified to the default value, "stop". > > > > > > > > I attached a very small patch to fix this just for > > > > making the description more accurate. > > > > > > Thank you for the patch! > > > > > > +1 to fix it. > > > > > > -ON_ERROR is not specified. This > > > -should not lead to problems in the event of a COPY > > > +ON_ERROR is not specified or > > > stop. > > > +This should not lead to problems in the event of a COPY > > > > > > How about the followings for consistency with the description of the > > > ON_ERROR option? > > > > > > COPY stops operation at the first error if the stop value is specified > > > to the ON_ERROR option. This should not lead to ... > > > > Thank you for you review. However, after posting the previous patch, > > I noticed that I overlooked ON_ERROR works only for errors due to data > > type incompatibility (that is mentioned as "malformed data" in the > > ON_ERROR description, though). > > Right. > > > > > So, how about rewriting this like: > > > > COPY stops operation at the first error unless the error is due to data > > type incompatibility and a value other than stop is specified to the > > ON_ERROR option. > > Hmm, this sentence seems not very readable to me, especially the "COPY > stops ... unless ... a value other than stop is specified ..." part. I > think we can simplify it: I can agree with your opinion on the readability, but... > COPY stops operation at the first data type incompatibility error if > the stop value is specified to the ON_ERROR option. This statement doesn't explain COPY also stops when an error other than data type incompatibility (e.g. constrain violations) occurs. Maybe, we can separate the sentese to two, for example: COPY stops operation at the first error. (The exception is if the error is due to data type incompatibility and a value other than stop is specified to the ON_ERROR option.) Regards, Yugo Nagata > Regards, > > -- > Masahiko Sawada > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 13:59:09 +0900 Masahiko Sawada wrote: > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA wrote: > > > > Hi, > > > > I found that the documentation of COPY ON_ERROR said > > COPY stops operation at the first error when > > "ON_ERROR is not specified.", but it also stop when > > ON_ERROR is specified to the default value, "stop". > > > > I attached a very small patch to fix this just for > > making the description more accurate. > > Thank you for the patch! > > +1 to fix it. > > -ON_ERROR is not specified. This > -should not lead to problems in the event of a COPY > +ON_ERROR is not specified or stop. > +This should not lead to problems in the event of a COPY > > How about the followings for consistency with the description of the > ON_ERROR option? > > COPY stops operation at the first error if the stop value is specified > to the ON_ERROR option. This should not lead to ... Thank you for you review. However, after posting the previous patch, I noticed that I overlooked ON_ERROR works only for errors due to data type incompatibility (that is mentioned as "malformed data" in the ON_ERROR description, though). So, how about rewriting this like: COPY stops operation at the first error unless the error is due to data type incompatibility and a value other than stop is specified to the ON_ERROR option. I attached the updated patch. Regards, Yugo Nagata > > Regards, > > -- > Masahiko Sawada > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 21a5c4a052..3676bf0e87 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -576,9 +576,10 @@ COPY count -COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY +COPY stops operation at the first error unless the error +is due to data type incompatibility and a value other than +stop is specified to the ON_ERROR option. +This should not lead to problems in the event of a COPY TO, but the target table will already have received earlier rows in a COPY FROM. These rows will not be visible or accessible, but they still occupy disk space. This might
Small fix on COPY ON_ERROR document
Hi, I found that the documentation of COPY ON_ERROR said COPY stops operation at the first error when "ON_ERROR is not specified.", but it also stop when ON_ERROR is specified to the default value, "stop". I attached a very small patch to fix this just for making the description more accurate. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 21a5c4a052..14c8ceab06 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -577,8 +577,8 @@ COPY count COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY +ON_ERROR is not specified or stop. +This should not lead to problems in the event of a COPY TO, but the target table will already have received earlier rows in a COPY FROM. These rows will not be visible or accessible, but they still occupy disk space. This might
Rename setup_cancel_handler in pg_dump
Hi,
Attached is a simple patch to rename setup_cancel_handler()
in pg_dump/parallel.c.
I am proposing it because there is a public function with
the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
does not include fe_utils/cancel.h, so there is no conflict,
but I think it is better to use different names to reduce
possible confusion.
I guess there was no concerns when setup_cancel_handler in
pg_dump/parallel.c was introduced because the same name
function was not in fe_utils that could be used in common
between client tools.. The public setup_cancel_handler in
fe_utils was introduced in a4fd3aa719e, where this function
was moved from psql.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..261b23cb3f 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void pg_dump_setup_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that pg_dump_setup_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ pg_dump_setup_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 2 Jan 2024 08:00:00 +0800
jian he wrote:
> On Mon, Nov 6, 2023 at 8:00 AM jian he wrote:
> >
> > minor doc issues.
> > Returns the chunk id of the TOASTed value, or NULL if the value is not
> > TOASTed.
> > Should it be "chunk_id"?
Thank you for your suggestion. As you pointed out, it is called "chunk_id"
in the documentation, so I rewrote it and also added a link to the section
where the TOAST table structure is explained.
> > you may place it after pg_create_logical_replication_slot entry to
> > make it look like alphabetical order.
I've been thinking about where we should place the function in the doc,
and I decided place it in the table of Database Object Size Functions
because I think pg_column_toast_chunk_id also would assist understanding
the result of size functions as similar to pg_column_compression; that is,
those function can explain why a large value in size could be stored in
a column.
> > There is no test. maybe we can add following to
> > src/test/regress/sql/misc.sql
> > create table val(t text);
> > INSERT into val(t) SELECT string_agg(
> > chr((ascii('B') + round(random() * 25)) :: integer),'')
> > FROM generate_series(1,2500);
> > select pg_column_toast_chunk_id(t) is not null from val;
> > drop table val;
Thank you for the test proposal. However, if we add a test, I want
to check that the chunk_id returned by the function exists in the
TOAST table, and that it returns NULL if the values is not TOASTed.
For the purpose, I wrote a test using a dynamic SQL since the table
name of the TOAST table have to be generated from the main table's OID.
> Hi
> the main C function (pg_column_toast_chunk_id) I didn't change.
> I added tests as mentioned above.
> tests put it on src/test/regress/sql/misc.sql, i hope that's fine.
> I placed pg_column_toast_chunk_id in "Table 9.99. Database Object
> Location Functions" (below Table 9.98. Database Object Size
> Functions).
I could not find any change in your patch from my previous patch.
Maybe, you attached wrong file. I attached a patch updated based
on your review, including the documentation fixes and a test.
What do you think about this it?
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 97af1b2300ecf07a34923da87a9d84e6aa963956 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v3] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17 ++
src/backend/utils/adt/varlena.c | 41 +
src/include/catalog/pg_proc.dat | 3 +++
3 files changed, 61 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 210c7c0b02..2d82331323 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN
Re: pgbnech: allow to cancel queries during benchmark
On Fri, 19 Jan 2024 17:46:03 +0900 (JST)
Tatsuo Ishii wrote:
> >> +/* send cancel requests to all connections */
> >> +static void
> >> +cancel_all()
> >> +{
> >> + for (int i = 0; i < nclients; i++)
> >> + {
> >> + char errbuf[1];
> >> + if (client_states[i].cancel != NULL)
> >> + (void) PQcancel(client_states[i].cancel, errbuf,
> >> sizeof(errbuf));
> >> + }
> >> +}
> >> +
> >>
> >> Why in case of errors from PQCancel the error message is neglected? I
> >> think it's better to print out the error message in case of error.
> >
> > Is the message useful for pgbench users? I saw the error is ignored
> > in pg_dump, for example in bin/pg_dump/parallel.c
>
> I think the situation is different from pg_dump. Unlike pg_dump, if
> PQcancel does not work, users can fix the problem by using
> pg_terminate_backend or kill command. In order to make this work, an
> appropriate error message is essential.
Makes sense. I fixed to emit an error message when PQcancel fails.
Also, I added some comments about the signal handling on Windows
to explain why the different way than non-Windows is required;
+* On Windows, a callback function is set in which query cancel requests
+* are sent to all benchmark queries running in the backend. This is
+* required because all threads running queries continue to run without
+* interrupted even when the signal is received.
+*
Attached is the updated patch, v6.
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
>
>
--
Yugo NAGATA
>From 52579f3d31a2927d8818953fabf8a908466e4fcf Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 24 Jul 2023 21:53:28 +0900
Subject: [PATCH v6] Allow pgbnech to cancel queries during benchmark
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries were not cancelled nd they keep on running on the
backend until they tried to send the result to pgbench.
The commit fixes this so that cancel requests are sent to all
connections before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c| 94
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +
2 files changed, 136 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 2574454839..e69c4af68a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3646,6 +3653,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4677,6 +4685,21 @@ disconnect_all(CState *state, int length)
finishCon([i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[256];
+ if (client_states[i].cancel != NULL)
+ {
+ if (!PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf)))
+pg_log_error("Could not send cancel request: %s", errbuf);
+ }
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7153,6 +7176,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7365,6 +7391,39 @@ threa
Re: Incremental View Maintenance, take 2
On Mon, 22 Jan 2024 13:51:08 +1100 Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > like there was some CFbot test failure last time it was run [2]. > Please have a look and post an updated version if necessary. Thank you for pointing out it. The CFbot failure is caused by a post [1] not by my patch-set, but regardless of it, I will heck if we need rebase and send the new version if necessary soon. [1] https://www.postgresql.org/message-id/CACJufxEoCCJE1vntJp1SWjen8vBUa3vZLgL%3DswPwar4zim976g%40mail.gmail.com Regards, Yugo Nagata > == > [1] https://commitfest.postgresql.org/46/4337/ > [2] https://cirrus-ci.com/task/6607979311529984 > > Kind Regards, > Peter Smith. -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
On Mon, 15 Jan 2024 16:49:44 +0900 (JST)
Tatsuo Ishii wrote:
> > On Wed, 6 Sep 2023 20:13:34 +0900
> > Yugo NAGATA wrote:
> >
> >> I attached the updated patch v3. The changes since the previous
> >> patch includes the following;
> >>
> >> I removed the unnecessary condition (&& false) that you
> >> pointed out in [1].
> >>
> >> The test was rewritten by using IPC::Run signal() and integrated
> >> to "001_pgbench_with_server.pl". This test is skipped on Windows
> >> because SIGINT causes to terminate the test itself as discussed
> >> in [2] about query cancellation test in psql.
> >>
> >> I added some comments to describe how query cancellation is
> >> handled as I explained in [1].
> >>
> >> Also, I found the previous patch didn't work on Windows so fixed it.
> >> On non-Windows system, a thread waiting a response of long query can
> >> be interrupted by SIGINT, but on Windows, threads do not return from
> >> waiting until queries they are running are cancelled. This is because,
> >> when the signal is received, the system just creates a new thread to
> >> execute the callback function specified by setup_cancel_handler, and
> >> other thread continue to run[3]. Therefore, the queries have to be
> >> cancelled in the callback function.
> >>
> >> [1]
> >> https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr
> >> [2]
> >> https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp
> >> [3] https://learn.microsoft.com/en-us/windows/console/handlerroutine
> >
> > I found that --disable-thread-safety option was removed in 68a4b58eca0329.
> > So, I removed codes involving ENABLE_THREAD_SAFETY from the patch.
> >
> > Also, I wrote a commit log draft.
>
> > Previously, Ctrl+C during benchmark killed pgbench immediately,
> > but queries running at that time were not cancelled.
>
> Better to mention explicitely that queries keep on running on the
> backend. What about this?
>
> Previously, Ctrl+C during benchmark killed pgbench immediately, but
> queries were not canceled and they keep on running on the backend
> until they tried to send the result to pgbench.
Thank you for your comments. I agree with you, so I fixed the message
as your suggestion.
> > The commit
> > fixes this so that cancel requests are sent for all connections
> > before pgbench exits.
>
> "sent for" -> "sent to"
Fixed.
> > Attached is the updated version, v4.
>
> +/* send cancel requests to all connections */
> +static void
> +cancel_all()
> +{
> + for (int i = 0; i < nclients; i++)
> + {
> + char errbuf[1];
> + if (client_states[i].cancel != NULL)
> + (void) PQcancel(client_states[i].cancel, errbuf,
> sizeof(errbuf));
> + }
> +}
> +
>
> Why in case of errors from PQCancel the error message is neglected? I
> think it's better to print out the error message in case of error.
Is the message useful for pgbench users? I saw the error is ignored
in pg_dump, for example in bin/pg_dump/parallel.c
/*
* Send QueryCancel to leader connection, if enabled. Ignore errors,
* there's not much we can do about them anyway.
*/
if (signal_info.myAH != NULL && signal_info.myAH->connCancel != NULL)
(void) PQcancel(signal_info.myAH->connCancel,
errbuf, sizeof(errbuf));
> + * On non-Windows, any callback function is not set. When SIGINT is
> + * received, CancelRequested is just set, and only thread #0 is
> interrupted
> + * and returns from waiting input from the backend. After that, the
> thread
> + * sends cancel requests to all benchmark queries.
>
> The second line is a little bit long according to the coding
> standard. Fix like this?
>
> * On non-Windows, any callback function is not set. When SIGINT is
>* received, CancelRequested is just set, and only thread #0 is
>* interrupted and returns from waiting input from the backend. After
>* that, the thread sends cancel requests to all benchmark queries.
Fixed.
The attached is the updated patch, v5.
Regards,
Yugo Nagata
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA
>From a42e6d47b27f2e91a02c5d934509070bd41bf79b Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 24 Jul 2023 21:53:28
doc: a small improvement about pg_am description
Hi, When reading the documentation about operator class, I found the following description: The pg_am table contains one row for every index access method. Support for access to regular tables is built into PostgreSQL, but all index access methods are described in pg_am. It seems to me that this description says pg_am contains only index access methods but not table methods. I wonder it is missed to fix this when tableam was supported and other documentation was changed in b73c3a11963c8bb783993cfffabb09f558f86e37. Attached is a patch to remove the sentence that starts with "Support for access to regular tables is ". Ragards, Yugo Nagata -- Yugo NAGATA diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml index c753d8005a..ff73233818 100644 --- a/doc/src/sgml/xindex.sgml +++ b/doc/src/sgml/xindex.sgml @@ -30,11 +30,8 @@ The pg_am table contains one row for every - index method (internally known as access method). Support for - regular access to tables is built into - PostgreSQL, but all index methods are - described in pg_am. It is possible to add a - new index access method by writing the necessary code and + index and table method (internally known as access method). It is possible + to add a new index access method by writing the necessary code and then creating an entry in pg_am but that is beyond the scope of this chapter (see ).
Re: Doc: vcregress .bat commands list lacks "taptest"
On Tue, 26 Sep 2023 08:18:01 +0900 Michael Paquier wrote: > On Mon, Sep 25, 2023 at 11:07:57AM -0400, Andrew Dunstan wrote: > > +1 > > Thanks, applied and backpatched then. Thank you! Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA
Doc: vcregress .bat commands list lacks "taptest"
Hi,
I found that "taptest" is missing in vcregress.bat command list in the
documentation when I tested psql and pgbench on Windows. I know there
is a plan to remove MSVC scripts[1], but is it worth adding one line
to the list for now?
[1] https://www.postgresql.org/message-id/flat/ZQzp_VMJcerM1Cs_%40paquier.xyz
I attached a patch for this case.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index 379a2ea80b..435a91228a 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -463,6 +463,7 @@ $ENV{CONFIG}="Debug";
vcregress isolationcheck
vcregress bincheck
vcregress recoverycheck
+vcregress taptest
To change the schedule used (default is parallel), append it to the
@@ -477,8 +478,9 @@ $ENV{CONFIG}="Debug";
Running the regression tests on client programs, with
- vcregress bincheck, or on recovery tests, with
- vcregress recoverycheck, requires an additional Perl module
+ vcregress bincheck, on recovery tests, with
+ vcregress recoverycheck, or TAP tests specified with
+ vcregress taptest requires an additional Perl module
to be installed:
Re: pgbnech: allow to cancel queries during benchmark
On Wed, 6 Sep 2023 20:13:34 +0900
Yugo NAGATA wrote:
> I attached the updated patch v3. The changes since the previous
> patch includes the following;
>
> I removed the unnecessary condition (&& false) that you
> pointed out in [1].
>
> The test was rewritten by using IPC::Run signal() and integrated
> to "001_pgbench_with_server.pl". This test is skipped on Windows
> because SIGINT causes to terminate the test itself as discussed
> in [2] about query cancellation test in psql.
>
> I added some comments to describe how query cancellation is
> handled as I explained in [1].
>
> Also, I found the previous patch didn't work on Windows so fixed it.
> On non-Windows system, a thread waiting a response of long query can
> be interrupted by SIGINT, but on Windows, threads do not return from
> waiting until queries they are running are cancelled. This is because,
> when the signal is received, the system just creates a new thread to
> execute the callback function specified by setup_cancel_handler, and
> other thread continue to run[3]. Therefore, the queries have to be
> cancelled in the callback function.
>
> [1]
> https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr
> [2]
> https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp
> [3] https://learn.microsoft.com/en-us/windows/console/handlerroutine
I found that --disable-thread-safety option was removed in 68a4b58eca0329.
So, I removed codes involving ENABLE_THREAD_SAFETY from the patch.
Also, I wrote a commit log draft.
Attached is the updated version, v4.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 10578e5cf02b1a0f7f0988bc7a877dcbeb5448b6 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 24 Jul 2023 21:53:28 +0900
Subject: [PATCH v4] Allow pgbnech to cancel queries during benchmark
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries running at that time were not cancelled. The commit
fixes this so that cancel requests are sent for all connections
before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c| 89
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +
2 files changed, 131 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..5adf099b76 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3639,6 +3646,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4678,18 @@ disconnect_all(CState *state, int length)
finishCon([i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7146,6 +7166,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7358,6 +7381,37 @@ threadRun(void *arg)
StatsData last,
aggs;
+ /*
+ * Query cancellation is handled only in thread #0.
+ *
+ * On Windows, a callback function is set in which query cancel requests
+ * are sent to all benchmark
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Wed, 13 Sep 2023 10:20:23 +0900 Michael Paquier wrote: > On Tue, Sep 12, 2023 at 03:18:05PM +0900, Michael Paquier wrote: > > On Wed, Sep 06, 2023 at 12:45:24AM +0900, Yugo NAGATA wrote: > > > I attached the update patch. I removed the incorrect comments and > > > unnecessary lines. Also, I rewrote the test to use "skip_all" instead > > > of SKIP because we skip the whole test rather than a part of it. > > > > Thanks for checking how IPC::Run behaves in this case on Windows! > > > > Right. This test is currently setting up a node for nothing, so let's > > skip this test entirely under $windows_os and move on. I'll backpatch > > that down to 15 once the embargo on REL_16_STABLE is lifted with the > > 16.0 tag. > > At the end, I have split this change into two: > - One to disable the test to run on Windows, skipping the wasted node > initialization, and applied that down to 15. > - One to switch to signal(), only for HEAD to see what happens in the > buildfarm once the test is able to run on platforms that do not > support PPID. I am wondering as well how IPC::Run::signal is stable, > as it is the first time we would use it, AFAIK. Thank you for pushing them. I agree with the suspection about IPC::Run::singnal, so I'll also check the buildfarm result. Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA
Re: psql help message contains excessive indentations
On Thu, 7 Sep 2023 13:06:35 +0200
Alvaro Herrera wrote:
> On 2023-Sep-07, Yugo NAGATA wrote:
>
> > Yes. So, I mean how about fixing \watch description as the attached patch.
>
> > diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
> > index 38c165a627..12280c0e54 100644
> > --- a/src/bin/psql/help.c
> > +++ b/src/bin/psql/help.c
> > @@ -200,9 +200,9 @@ slashUsage(unsigned short int pager)
> > HELP0(" \\gset [PREFIX] execute query and store result in psql
> > variables\n");
> > HELP0(" \\gx [(OPTIONS)] [FILE] as \\g, but forces expanded output
> > mode\n");
> > HELP0(" \\q quit psql\n");
> > - HELP0(" \\watch [[i=]SEC] [c=N] [m=MIN]\n");
> > - HELP0(" execute query every SEC seconds, up to
> > N times\n");
> > - HELP0(" stop if less than MIN rows are
> > returned\n");
> > + HELP0(" \\watch [[i=]SEC] [c=N] [m=MIN]\n"
> > + " execute query every SEC seconds, up
> > to N times\n"
> > + " stop if less than MIN rows are
> > returned\n");
>
> Yeah, this looks right to me -- the whole help entry as a single
> translatable unit, instead of three separately translatable lines.
Thank you for your explanation. I understood it. I thought of just
imitating other places, and I didn't know each is a single translatable
unit.
Regards,
Yugo Nagata
> --
> Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
> "Estoy de acuerdo contigo en que la verdad absoluta no existe...
> El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)
--
Yugo NAGATA
Re: psql help message contains excessive indentations
On Thu, 07 Sep 2023 15:36:10 +0900 (JST)
Kyotaro Horiguchi wrote:
> At Thu, 7 Sep 2023 15:02:49 +0900, Yugo NAGATA wrote in
> > On Thu, 07 Sep 2023 14:29:56 +0900 (JST)
> > Kyotaro Horiguchi wrote:
> > > > \q quit psql
> > > > \watch [[i=]SEC] [c=N] [m=MIN]
> > > !> execute query every SEC seconds, up to N times
> > > !> stop if less than MIN rows are returned
> > >
> > > The last two lines definitely have some extra indentation.
> >
> > Agreed.
> >
> > > I've attached a patch that fixes this.
> >
> > I wonder this better to fix this in similar way to other places where the
> > description has multiple lines., like "\g [(OPTIONS)] [FILE]".
>
> It looks correct to me:
Yes. So, I mean how about fixing \watch description as the attached patch.
>
> > \errverboseshow most recent error message at maximum verbosity
> > \g [(OPTIONS)] [FILE] execute query (and send result to file or |pipe);
> > \g with no arguments is equivalent to a semicolon
> > \gdesc describe result of query, without executing it
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
--
Yugo NAGATA
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 38c165a627..12280c0e54 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -200,9 +200,9 @@ slashUsage(unsigned short int pager)
HELP0(" \\gset [PREFIX] execute query and store result in psql variables\n");
HELP0(" \\gx [(OPTIONS)] [FILE] as \\g, but forces expanded output mode\n");
HELP0(" \\q quit psql\n");
- HELP0(" \\watch [[i=]SEC] [c=N] [m=MIN]\n");
- HELP0(" execute query every SEC seconds, up to N times\n");
- HELP0(" stop if less than MIN rows are returned\n");
+ HELP0(" \\watch [[i=]SEC] [c=N] [m=MIN]\n"
+ " execute query every SEC seconds, up to N times\n"
+ " stop if less than MIN rows are returned\n");
HELP0("\n");
HELP0("Help\n");
Re: psql help message contains excessive indentations
On Thu, 07 Sep 2023 14:29:56 +0900 (JST) Kyotaro Horiguchi wrote: > This topic is extracted from [1]. > > As mentioned there, in psql, running \? displays the following lines. > > > \gdesc describe result of query, without executing it > > \gexec execute query, then execute each value in its > result > > \gset [PREFIX] execute query and store result in psql variables > > \gx [(OPTIONS)] [FILE] as \g, but forces expanded output mode > > \q quit psql > > \watch [[i=]SEC] [c=N] [m=MIN] > !> execute query every SEC seconds, up to N times > !> stop if less than MIN rows are returned > > The last two lines definitely have some extra indentation. Agreed. > I've attached a patch that fixes this. I wonder this better to fix this in similar way to other places where the description has multiple lines., like "\g [(OPTIONS)] [FILE]". Regards, Yugo Nagata > > [1] > https://www.postgresql.org/message-id/20230830.103356.1909699532104167477.horikyota@gmail.com > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
Hi Fabien,
On Thu, 10 Aug 2023 12:32:44 +0900
Yugo NAGATA wrote:
> On Wed, 9 Aug 2023 11:18:43 +0200 (CEST)
> Fabien COELHO wrote:
>
> >
> > I forgot, about the test:
> >
> > I think that it should be integrated in the existing
> > "001_pgbench_with_server.pl" script, because a TAP script is pretty
> > expensive as it creates a cluster, starts it… before running the test.
>
> Ok. I'll integrate the test into 001.
>
> > I'm surprise that IPC::Run does not give any access to the process number.
> > Anyway, its object interface seems to allow sending signal:
> >
> > $h->signal("...")
> >
> > So the code could be simplified to use that after a small delay.
>
> Thank you for your information.
>
> I didn't know $h->signal() and I mimicked the way of
> src/bin/psql/t/020_cancel.pl to send SIGINT to a running program. I don't
> know why the psql test doesn't use the interface, I'll investigate whether
> this can be used in our purpose, anyway.
I attached the updated patch v3. The changes since the previous
patch includes the following;
I removed the unnecessary condition (&& false) that you
pointed out in [1].
The test was rewritten by using IPC::Run signal() and integrated
to "001_pgbench_with_server.pl". This test is skipped on Windows
because SIGINT causes to terminate the test itself as discussed
in [2] about query cancellation test in psql.
I added some comments to describe how query cancellation is
handled as I explained in [1].
Also, I found the previous patch didn't work on Windows so fixed it.
On non-Windows system, a thread waiting a response of long query can
be interrupted by SIGINT, but on Windows, threads do not return from
waiting until queries they are running are cancelled. This is because,
when the signal is received, the system just creates a new thread to
execute the callback function specified by setup_cancel_handler, and
other thread continue to run[3]. Therefore, the queries have to be
cancelled in the callback function.
[1]
https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr
[2]
https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp
[3] https://learn.microsoft.com/en-us/windows/console/handlerroutine
Regards,
Yugo Nagata
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 713e8a06bb..1e396865f7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3639,6 +3646,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4678,18 @@ disconnect_all(CState *state, int length)
finishCon([i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7146,6 +7166,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7358,6 +7381,37 @@ threadRun(void *arg)
StatsData last,
aggs;
+ /*
+ * Query cancellation is handled only in thread #0.
+ *
+ * On Windows, a callback function is set in which query cancel requests
+ * are sent to all benchmark queries running in the backend.
+ *
+ * On non-Windows, any callback function is not set. When SIGINT is
+ * received, CancelRequested is just set, and only thread #0 is interrupted
+ * and returns from waiting input from the backend. After that, the thread
+ * sends cancel requests to all benchmark queries.
+
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Mon, 14 Aug 2023 23:37:25 +0900
Yugo NAGATA wrote:
> On Mon, 14 Aug 2023 08:29:25 +0900
> Michael Paquier wrote:
>
> > On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
> > > Test run is ok on my Ubuntu laptop.
> >
> > I have a few comments about this patch.
> >
> > On HEAD and even after this patch, we still have the following:
> > SKIP:
> >
> >{
> >
> > skip "cancel test requires a Unix
> > shell", 2 if $windows_os;
> >
> > Could the SKIP be removed for $windows_os? If not, this had better be
> > documented because the reason for the skip becomes incorrect.
> >
> > The comment at the top of the SKIP block still states the following:
> > # There is, as of this writing, no documented way to get the PID of
> > # the process from IPC::Run. As a workaround, we have psql print its
> > # own PID (which is the parent of the shell launched by psql) to a
> > # file.
> >
> > This is also incorrect.
>
> Thank you for your comments
>
> I will check whether the test works in Windows and remove SKIP if possible.
> Also, I'll fix the comment in either case.
I checked if the test using IPC::Run::signal can work on Windows, and
confirmed that this didn't work because sending SIGINT caused to
terminate the test itself. Here is the results;
t/001_basic.pl ... ok
t/010_tab_completion.pl .. skipped: readline is not supported by this build
t/020_cancel.pl .. Terminating on signal SIGINT(2)
Therefore, this test should be skipped on Windows.
I attached the update patch. I removed the incorrect comments and
unnecessary lines. Also, I rewrote the test to use "skip_all" instead
of SKIP because we skip the whole test rather than a part of it.
Regards,
Yugo Nagata
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
index 0765d82b92..cabbf0210a 100644
--- a/src/bin/psql/t/020_cancel.pl
+++ b/src/bin/psql/t/020_cancel.pl
@@ -9,72 +9,39 @@ use PostgreSQL::Test::Utils;
use Test::More;
use Time::HiRes qw(usleep);
-my $tempdir = PostgreSQL::Test::Utils::tempdir;
+# Test query canceling by sending SIGINT to a running psql
+
+if ($windows_os)
+{
+ plan skip_all => 'sending SIGINT on Windows terminates the test itself';
+}
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->start;
-# Test query canceling by sending SIGINT to a running psql
-#
-# There is, as of this writing, no documented way to get the PID of
-# the process from IPC::Run. As a workaround, we have psql print its
-# own PID (which is the parent of the shell launched by psql) to a
-# file.
-SKIP:
-{
- skip "cancel test requires a Unix shell", 2 if $windows_os;
+local %ENV = $node->_get_env();
- local %ENV = $node->_get_env();
+my ($stdin, $stdout, $stderr);
+my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
+ \$stdin, \$stdout, \$stderr);
- my ($stdin, $stdout, $stderr);
+# Send sleep command and wait until the server has registered it
+$stdin = "select pg_sleep($PostgreSQL::Test::Utils::timeout_default);\n";
+pump $h while length $stdin;
+$node->poll_query_until('postgres',
+ q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ '^select pg_sleep') > 0;}
+) or die "timed out";
- # Test whether shell supports $PPID. It's part of POSIX, but some
- # pre-/non-POSIX shells don't support it (e.g., NetBSD).
- $stdin = "\\! echo \$PPID";
- IPC::Run::run([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- '<', \$stdin, '>', \$stdout, '2>', \$stderr);
- $stdout =~ /^\d+$/ or skip "shell apparently does not support \$PPID", 2;
+# Send cancel request
+$h->signal('INT');
- # Now start the real test
- my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- \$stdin, \$stdout, \$stderr);
+my $result = finish $h;
- # Get the PID
- $stdout = '';
- $stderr = '';
- $stdin = "\\! echo \$PPID >$tempdir/psql.pid\n";
- pump $h while length $stdin;
- my $count;
- my $psql_pid;
- until (
- -s "$tempdir/psql.pid"
- and ($psql_pid =
- PostgreSQL::Test::Utils::slurp_file("$tempdir/psql.pid")) =~
- /^\d+\n/s)
- {
- ($count++ < 100 * $PostgreSQL::Test::Utils::timeout_default)
- or die "pid file did not appear";
- usleep(10_000);
- }
-
- # Send sleep command and wait until the server ha
Re: pgbench: allow to exit immediately when any client is aborted
On Wed, 30 Aug 2023 10:11:10 +0900 (JST) Tatsuo Ishii wrote: > Yugo, > Fabien, > > >>> I start to think this behavior is ok and consistent with previous > >>> behavior of pgbench because serialization (and dealock) errors have > >>> been treated specially from other types of errors, such as accessing > >>> non existing tables. However, I suggest to add more sentences to the > >>> explanation of this option so that users are not confused by this. > >>> > >>> + > >>> + --exit-on-abort > >>> + > >>> + > >>> +Exit immediately when any client is aborted due to some error. > >>> Without > >>> +this option, even when a client is aborted, other clients could > >>> continue > >>> +their run as specified by -t or > >>> -T option, > >>> +and pgbench will print an incomplete > >>> results > >>> +in this case. > >>> + > >>> + > >>> + > >>> + > >>> > >>> What about inserting "Note that serialization failures or deadlock > >>> failures will not abort client. See >>> linkend="failures-and-retries"/> for more information." into the end > >>> of this paragraph? > >> > >> --exit-on-abort is related to "abort" of a client instead of error or > >> failure itself, so rather I wonder a bit that mentioning > >> serialization/deadlock > >> failures might be confusing. However, if users may think of such failures > >> from > >> "abort", it could be beneficial to add the sentences with some > >> modification as > >> below. > > > > I myself confused by this and believe that adding extra paragraph is > > beneficial to users. > > > >> "Note that serialization failures or deadlock failures does not abort the > >> client, so they are not affected by this option. > >> See for more information." > > > > "does not" --> "do not". > > > >>> BTW, I think: > >>> Exit immediately when any client is aborted due to some error. > >>> Without > >>> > >>> should be: > >>> Exit immediately when any client is aborted due to some errors. > >>> Without > >>> > >>> (error vs. erros) > >> > >> Well, I chose "some" to mean "unknown or unspecified", not "an unspecified > >> amount > >> or number of", so singular form "error" is used. > > > > Ok. > > > >> Instead, should we use "due to a error"? > > > > I don't think so. > > > >>> Also: > >>> + --exit-on-abort is specified . Otherwise in > >>> the worst > >>> > >>> There is an extra space between "specified" and ".". > >> > >> Fixed. > >> > >> Also, I fixed the place of the description in the documentation > >> to alphabetical order > >> > >> Attached is the updated patch. > > > > Looks good to me. If there's no objection, I will commit this next week. > > I have pushed the patch. Thank you for the conributions! Thank you! Regards, Yugo Nagata > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Sun, 2 Jul 2023 10:38:20 +0800 jian he wrote: > ok. Now I really found a small bug. > > this works as intended: > BEGIN; > CREATE INCREMENTAL MATERIALIZED VIEW test_ivm AS SELECT i, MIN(j) as > min_j FROM mv_base_a group by 1; > INSERT INTO mv_base_a select 1,-2 where false; > rollback; > > however the following one: > BEGIN; > CREATE INCREMENTAL MATERIALIZED VIEW test_ivm1 AS SELECT MIN(j) as > min_j FROM mv_base_a; > INSERT INTO mv_base_a select 1, -2 where false; > rollback; > > will evaluate > tuplestore_tuple_count(new_tuplestores) to 1, it will walk through > IVM_immediate_maintenance function to apply_delta. > but should it be zero? This is not a bug because an aggregate without GROUP BY always results one row whose value is NULL. The contents of test_imv1 would be always same as " SELECT MIN(j) as min_j FROM mv_base_a;", isn't it? Regards, Yugo Nagata -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Sun, 2 Jul 2023 08:25:12 +0800
jian he wrote:
> This is probably not trivial.
> In function apply_new_delta_with_count.
>
> appendStringInfo(,
> "WITH updt AS (" /* update a tuple if this exists in the view */
> "UPDATE %s AS mv SET %s = mv.%s OPERATOR(pg_catalog.+) diff.%s "
> "%s " /* SET clauses for aggregates */
> "FROM %s AS diff "
> "WHERE %s " /* tuple matching condition */
> "RETURNING %s" /* returning keys of updated tuples */
> ") INSERT INTO %s (%s)" /* insert a new tuple if this doesn't existw */
> "SELECT %s FROM %s AS diff "
> "WHERE NOT EXISTS (SELECT 1 FROM updt AS mv WHERE %s);",
>
> -
> ") INSERT INTO %s (%s)" /* insert a new tuple if this doesn't existw */
> "SELECT %s FROM %s AS diff "
>
> the INSERT INTO line, should have one white space in the end?
> also "existw" should be "exists"
Yes, we should need a space although it works. I'll fix as well as the typo.
Thank you.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Fri, 30 Jun 2023 08:00:00 +0800 jian he wrote: > Hi there. > in v28-0005-Add-Incremental-View-Maintenance-support-to-psql.patch > I don't know how to set psql to get the output > "Incremental view maintenance: yes" This information will appear when you use "d+" command for an incrementally maintained materialized view. Regards, Yugo Nagata -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 18:51:06 +0800
jian he wrote:
> I cannot build the doc.
> git clean -fdx
> git am ~/Desktop/tmp/*.patch
>
> Applying: Add a syntax to create Incrementally Maintainable Materialized Views
> Applying: Add relisivm column to pg_class system catalog
> Applying: Allow to prolong life span of transition tables until transaction
> end
> Applying: Add Incremental View Maintenance support to pg_dump
> Applying: Add Incremental View Maintenance support to psql
> Applying: Add Incremental View Maintenance support
> Applying: Add DISTINCT support for IVM
> Applying: Add aggregates support in IVM
> Applying: Add support for min/max aggregates for IVM
> Applying: Add regression tests for Incremental View Maintenance
> Applying: Add documentations about Incremental View Maintenance
> .git/rebase-apply/patch:79: trailing whitespace.
> clause.
> warning: 1 line adds whitespace errors.
>
> Because of this, the {ninja docs} command failed. ERROR message:
>
> [6/6] Generating doc/src/sgml/html with a custom command
> FAILED: doc/src/sgml/html
> /usr/bin/python3
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml/xmltools_dep_wrapper
> --targetname doc/src/sgml/html --depfile doc/src/sgml/html.d --tool
> /usr/bin/xsltproc -- -o doc/src/sgml/ --nonet --stringparam pg.version
> 16beta2 --path doc/src/sgml --path
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml/stylesheet.xsl
> doc/src/sgml/postgres-full.xml
> ERROR: id attribute missing on element under /book[@id =
> 'postgres']/part[@id = 'server-programming']/chapter[@id =
> 'rules']/sect1[@id = 'rules-ivm']
> error: file doc/src/sgml/postgres-full.xml
> xsltRunStylesheet : run failed
> ninja: build stopped: subcommand failed.
Thank your for pointing out this.
I'll add ids for all sections to suppress the errors.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 18:20:32 +0800 jian he wrote: > On Thu, Jun 29, 2023 at 12:40 AM jian he wrote: > > > > On Wed, Jun 28, 2023 at 4:06 PM Yugo NAGATA wrote: > > > > > > On Wed, 28 Jun 2023 00:01:02 +0800 > > > jian he wrote: > > > > > > > On Thu, Jun 1, 2023 at 2:47 AM Yugo NAGATA wrote: > > > > > > > > > > On Thu, 1 Jun 2023 23:59:09 +0900 > > > > > Yugo NAGATA wrote: > > > > > > > > > > > Hello hackers, > > > > > > > > > > > > Here's a rebased version of the patch-set adding Incremental View > > > > > > Maintenance support for PostgreSQL. That was discussed in [1]. > > > > > > > > > > > [1] > > > > > > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp > > > > > > > > > > --- > > > > > * Overview > > > > > > > > > > Incremental View Maintenance (IVM) is a way to make materialized views > > > > > up-to-date by computing only incremental changes and applying them on > > > > > views. IVM is more efficient than REFRESH MATERIALIZED VIEW when > > > > > only small parts of the view are changed. > > > > > > > > > > ** Feature > > > > > > > > > > The attached patchset provides a feature that allows materialized > > > > > views > > > > > to be updated automatically and incrementally just after a underlying > > > > > table is modified. > > > > > > > > > > You can create an incementally maintainable materialized view (IMMV) > > > > > by using CREATE INCREMENTAL MATERIALIZED VIEW command. > > > > > > > > > > The followings are supported in view definition queries: > > > > > - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) > > > > > - some built-in aggregate functions (count, sum, avg, min, max) > > > > > - GROUP BY clause > > > > > - DISTINCT clause > > > > > > > > > > Views can contain multiple tuples with the same content (duplicate > > > > > tuples). > > > > > > > > > > ** Restriction > > > > > > > > > > The following are not supported in a view definition: > > > > > - Outer joins > > > > > - Aggregates otehr than above, window functions, HAVING > > > > > - Sub-queries, CTEs > > > > > - Set operations (UNION, INTERSECT, EXCEPT) > > > > > - DISTINCT ON, ORDER BY, LIMIT, OFFSET > > > > > > > > > > Also, a view definition query cannot contain other views, > > > > > materialized views, > > > > > foreign tables, partitioned tables, partitions, VALUES, non-immutable > > > > > functions, > > > > > system columns, or expressions that contains aggregates. > > > > > > > > > > --- > > > > > * Design > > > > > > > > > > An IMMV is maintained using statement-level AFTER triggers. > > > > > When an IMMV is created, triggers are automatically created on all > > > > > base > > > > > tables contained in the view definition query. > > > > > > > > > > When a table is modified, changes that occurred in the table are > > > > > extracted > > > > > as transition tables in the AFTER triggers. Then, changes that will > > > > > occur in > > > > > the view are calculated by a rewritten view dequery in which the > > > > > modified table > > > > > is replaced with the transition table. > > > > > > > > > > For example, if the view is defined as "SELECT * FROM R, S", and > > > > > tuples inserted > > > > > into R are stored in a transiton table dR, the tuples that will be > > > > > inserted into > > > > > the view are calculated as the result of "SELECT * FROM dR, S". > > > > > > > > > > ** Multiple Tables Modification > > > > > > > > > > Multiple tables can be modified in a statement when using triggers, > > > > > fore
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 00:40:45 +0800 jian he wrote: > On Wed, Jun 28, 2023 at 4:06 PM Yugo NAGATA wrote: > > > > On Wed, 28 Jun 2023 00:01:02 +0800 > > jian he wrote: > > > > > On Thu, Jun 1, 2023 at 2:47 AM Yugo NAGATA wrote: > > > > > > > > On Thu, 1 Jun 2023 23:59:09 +0900 > > > > Yugo NAGATA wrote: > > > > > > > > > Hello hackers, > > > > > > > > > > Here's a rebased version of the patch-set adding Incremental View > > > > > Maintenance support for PostgreSQL. That was discussed in [1]. > > > > > > > > > [1] > > > > > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp > > > > > > > > --- > > > > * Overview > > > > > > > > Incremental View Maintenance (IVM) is a way to make materialized views > > > > up-to-date by computing only incremental changes and applying them on > > > > views. IVM is more efficient than REFRESH MATERIALIZED VIEW when > > > > only small parts of the view are changed. > > > > > > > > ** Feature > > > > > > > > The attached patchset provides a feature that allows materialized views > > > > to be updated automatically and incrementally just after a underlying > > > > table is modified. > > > > > > > > You can create an incementally maintainable materialized view (IMMV) > > > > by using CREATE INCREMENTAL MATERIALIZED VIEW command. > > > > > > > > The followings are supported in view definition queries: > > > > - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) > > > > - some built-in aggregate functions (count, sum, avg, min, max) > > > > - GROUP BY clause > > > > - DISTINCT clause > > > > > > > > Views can contain multiple tuples with the same content (duplicate > > > > tuples). > > > > > > > > ** Restriction > > > > > > > > The following are not supported in a view definition: > > > > - Outer joins > > > > - Aggregates otehr than above, window functions, HAVING > > > > - Sub-queries, CTEs > > > > - Set operations (UNION, INTERSECT, EXCEPT) > > > > - DISTINCT ON, ORDER BY, LIMIT, OFFSET > > > > > > > > Also, a view definition query cannot contain other views, materialized > > > > views, > > > > foreign tables, partitioned tables, partitions, VALUES, non-immutable > > > > functions, > > > > system columns, or expressions that contains aggregates. > > > > > > > > --- > > > > * Design > > > > > > > > An IMMV is maintained using statement-level AFTER triggers. > > > > When an IMMV is created, triggers are automatically created on all base > > > > tables contained in the view definition query. > > > > > > > > When a table is modified, changes that occurred in the table are > > > > extracted > > > > as transition tables in the AFTER triggers. Then, changes that will > > > > occur in > > > > the view are calculated by a rewritten view dequery in which the > > > > modified table > > > > is replaced with the transition table. > > > > > > > > For example, if the view is defined as "SELECT * FROM R, S", and tuples > > > > inserted > > > > into R are stored in a transiton table dR, the tuples that will be > > > > inserted into > > > > the view are calculated as the result of "SELECT * FROM dR, S". > > > > > > > > ** Multiple Tables Modification > > > > > > > > Multiple tables can be modified in a statement when using triggers, > > > > foreign key > > > > constraint, or modifying CTEs. When multiple tables are modified, we > > > > need > > > > the state of tables before the modification. > > > > > > > > For example, when some tuples, dR and dS, are inserted into R and S > > > > respectively, > > > > the tuples that will be inserted into the view are calculated by the > > > > following > > > > two queries: > > > > > > > > "SEL
Re: pgbench: allow to exit immediately when any client is aborted
On Thu, 24 Aug 2023 09:15:51 +0900 (JST) Tatsuo Ishii wrote: > >> I start to think this behavior is ok and consistent with previous > >> behavior of pgbench because serialization (and dealock) errors have > >> been treated specially from other types of errors, such as accessing > >> non existing tables. However, I suggest to add more sentences to the > >> explanation of this option so that users are not confused by this. > >> > >> + > >> + --exit-on-abort > >> + > >> + > >> +Exit immediately when any client is aborted due to some error. > >> Without > >> +this option, even when a client is aborted, other clients could > >> continue > >> +their run as specified by -t or > >> -T option, > >> +and pgbench will print an incomplete > >> results > >> +in this case. > >> + > >> + > >> + > >> + > >> > >> What about inserting "Note that serialization failures or deadlock > >> failures will not abort client. See >> linkend="failures-and-retries"/> for more information." into the end > >> of this paragraph? > > > > --exit-on-abort is related to "abort" of a client instead of error or > > failure itself, so rather I wonder a bit that mentioning > > serialization/deadlock > > failures might be confusing. However, if users may think of such failures > > from > > "abort", it could be beneficial to add the sentences with some modification > > as > > below. > > I myself confused by this and believe that adding extra paragraph is > beneficial to users. Ok. > > "Note that serialization failures or deadlock failures does not abort the > > client, so they are not affected by this option. > > See for more information." > > "does not" --> "do not". Oops. I attached the updated patch. > >> BTW, I think: > >> Exit immediately when any client is aborted due to some error. > >> Without > >> > >> should be: > >> Exit immediately when any client is aborted due to some errors. > >> Without > >> > >> (error vs. erros) > > > > Well, I chose "some" to mean "unknown or unspecified", not "an unspecified > > amount > > or number of", so singular form "error" is used. > > Ok. > > > Instead, should we use "due to a error"? > > I don't think so. > > >> Also: > >> + --exit-on-abort is specified . Otherwise in the > >> worst > >> > >> There is an extra space between "specified" and ".". > > > > Fixed. > > > > Also, I fixed the place of the description in the documentation > > to alphabetical order > > > > Attached is the updated patch. > > Looks good to me. If there's no objection, I will commit this next week. > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 6c5c8afa6d..95cc9027c3 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -768,6 +768,24 @@ pgbench options d + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + +Note that serialization failures or deadlock failures do not abort the +client, so they are not affected by this option. +See for more information. + + + + --failures-detailed @@ -985,7 +1003,8 @@ pgbench options d benchmark such as initial connection failures also exit with status 1. Errors during the run such as database errors or problems in the script will result in exit status 2. In the latter case, - pgbench will print partial results. + pgbench will print partial results if + --exit-on-abort option is not specified. @@ -2801,14 +2820,17 @@ statement latencies in milliseconds, failures and retries: start a connection to the database server / the socket for connecting the client to the database server has become invalid). In
Re: pgbench: allow to exit immediately when any client is aborted
On Sat, 19 Aug 2023 19:51:56 +0900 (JST) Tatsuo Ishii wrote: > > I have tested the v4 patch with default_transaction_isolation = > > 'repeatable read'. > > > > pgbench --exit-on-abort -N -p 11002 -c 10 -T 30 test > > pgbench (17devel, server 15.3) > > starting vacuum...end. > > transaction type: > > scaling factor: 1 > > query mode: simple > > number of clients: 10 > > number of threads: 1 > > maximum number of tries: 1 > > duration: 30 s > > number of transactions actually processed: 64854 > > number of failed transactions: 4 (0.006%) > > latency average = 4.628 ms (including failures) > > initial connection time = 49.526 ms > > tps = 2160.827556 (without initial connection time) > > > > Depite the 4 failed transactions (could not serialize access due to > > concurrent update) pgbench ran normally, rather than aborting the > > test. Is this an expected behavior? Yes. --exit-on-abort changes a behaviour when a client is **aborted** due to an error, and serialization errors do not cause abort, so it is not affected. > I start to think this behavior is ok and consistent with previous > behavior of pgbench because serialization (and dealock) errors have > been treated specially from other types of errors, such as accessing > non existing tables. However, I suggest to add more sentences to the > explanation of this option so that users are not confused by this. > > + > + --exit-on-abort > + > + > +Exit immediately when any client is aborted due to some error. > Without > +this option, even when a client is aborted, other clients could > continue > +their run as specified by -t or -T > option, > +and pgbench will print an incomplete > results > +in this case. > + > + > + > + > > What about inserting "Note that serialization failures or deadlock > failures will not abort client. See linkend="failures-and-retries"/> for more information." into the end > of this paragraph? --exit-on-abort is related to "abort" of a client instead of error or failure itself, so rather I wonder a bit that mentioning serialization/deadlock failures might be confusing. However, if users may think of such failures from "abort", it could be beneficial to add the sentences with some modification as below. "Note that serialization failures or deadlock failures does not abort the client, so they are not affected by this option. See for more information." > BTW, I think: > Exit immediately when any client is aborted due to some error. Without > > should be: > Exit immediately when any client is aborted due to some errors. > Without > > (error vs. erros) Well, I chose "some" to mean "unknown or unspecified", not "an unspecified amount or number of", so singular form "error" is used. Instead, should we use "due to a error"? > Also: > + --exit-on-abort is specified . Otherwise in the > worst > > There is an extra space between "specified" and ".". Fixed. Also, I fixed the place of the description in the documentation to alphabetical order Attached is the updated patch. Regards, Yugo Nagata > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 6c5c8afa6d..4175cf4ccd 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -768,6 +768,24 @@ pgbench options d + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + +Note that serialization failures or deadlock failures does not abort the +client, so they are not affected by this option. +See for more information. + + + + --failures-detailed @@ -985,7 +1003,8 @@ pgbench options d benchmark such as initial connection failures also exit with status 1. Errors during the run such as database errors or problems in the script will result in exit status 2. In the latter case, - pgbench will print partial results. + pgbench will print partial results if + --exit-on-abort option is not specified. @@ -2801,14 +2820,17 @@ statement
Re: Make psql's qeury canceling test simple by using signal() routine of IPC::Run
On Mon, 14 Aug 2023 08:29:25 +0900
Michael Paquier wrote:
> On Sun, Aug 13, 2023 at 11:22:33AM +0200, Fabien COELHO wrote:
> > Test run is ok on my Ubuntu laptop.
>
> I have a few comments about this patch.
>
> On HEAD and even after this patch, we still have the following:
> SKIP:
>
>{
>
> skip "cancel test requires a Unix shell", 2
> if $windows_os;
>
> Could the SKIP be removed for $windows_os? If not, this had better be
> documented because the reason for the skip becomes incorrect.
>
> The comment at the top of the SKIP block still states the following:
> # There is, as of this writing, no documented way to get the PID of
> # the process from IPC::Run. As a workaround, we have psql print its
> # own PID (which is the parent of the shell launched by psql) to a
> # file.
>
> This is also incorrect.
Thank you for your comments
I will check whether the test works in Windows and remove SKIP if possible.
Also, I'll fix the comment in either case.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: pgbench: allow to exit immediately when any client is aborted
On Sun, 13 Aug 2023 11:27:55 +0200 (CEST)
Fabien COELHO wrote:
>
> Hello Yugo-san,
>
> >> I attached the updated patch v3 including changes above, a test,
> >> and fix of the typo you pointed out.
> >
> > I'm sorry but the test in the previous patch was incorrect.
> > I attached the correct one.
>
> About pgbench exit on abort v3:
>
> Patch does not "git apply", but is ok with "patch" although there are some
> minor warnings.
In my environment, the patch can be applied to the master branch without
any warnings...
>
> Compiles. Code is ok. Tests are ok.
>
> About Test:
>
> The code is simple to get an error quickly but after a few transactions,
> good. I'll do a minimal "-c 2 -j 2 -t 10" instead of "-c 4 -j 4 -T 10".
I fixed the test and attached the updated patch, v4.
Regards,
Yugo Nagata
>
> --
> Fabien.
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 6c5c8afa6d..83fe01c33f 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -787,6 +787,19 @@ pgbench options d
+
+ --exit-on-abort
+
+
+Exit immediately when any client is aborted due to some error. Without
+this option, even when a client is aborted, other clients could continue
+their run as specified by -t or -T option,
+and pgbench will print an incomplete results
+in this case.
+
+
+
+
--log-prefix=prefix
@@ -985,7 +998,8 @@ pgbench options d
benchmark such as initial connection failures also exit with status 1.
Errors during the run such as database errors or problems in the script
will result in exit status 2. In the latter case,
- pgbench will print partial results.
+ pgbench will print partial results if
+ --exit-on-abort option is not specified.
@@ -2801,14 +2815,17 @@ statement latencies in milliseconds, failures and retries:
start a connection to the database server / the socket for connecting
the client to the database server has become invalid). In such cases
all clients of this thread stop while other threads continue to work.
+ However, --exit-on-abort is specified, all of the
+ threads stop immediately in this case.
Direct client errors. They lead to immediate exit from
pgbench with the corresponding error message
- only in the case of an internal pgbench
- error (which are supposed to never occur...). Otherwise in the worst
+ in the case of an internal pgbench
+ error (which are supposed to never occur...) or when
+ --exit-on-abort is specified . Otherwise in the worst
case they only lead to the abortion of the failed client while other
clients continue their run (but some client errors are handled without
an abortion of the client and reported separately, see below). Later in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 763c4b946a..4e64a60a63 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -765,6 +765,8 @@ static int64 total_weight = 0;
static bool verbose_errors = false; /* print verbose messages of all errors */
+static bool exit_on_abort = false; /* exit when any client is aborted */
+
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -911,6 +913,7 @@ usage(void)
" -T, --time=NUM duration of benchmark test in seconds\n"
" -v, --vacuum-all vacuum all four standard tables before tests\n"
" --aggregate-interval=NUM aggregate data over NUM seconds\n"
+ " --exit-on-abort exit when any client is aborted\n"
" --failures-detailed report the failures grouped by basic types\n"
" --log-prefix=PREFIX prefix for transaction time log file\n"
" (default: \"pgbench_log\")\n"
@@ -6617,6 +6620,7 @@ main(int argc, char **argv)
{"failures-detailed", no_argument, NULL, 13},
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
+ {"exit-on-abort", no_argument, NULL, 16},
{NULL, 0, NULL, 0}
};
@@ -6950,6 +6954,10 @@ main(int argc, char **argv)
benchmarking_option_set = true;
verbose_errors = true;
break;
+ case 16: /* exit-on-abort */
+benchmarking_option_set = true;
+exit_on_abort = true;
+break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7558,11 +7566,17 @@ threadRun(void *arg)
advanceConnectionState(thr
Make psql's qeury canceling test simple by using signal() routine of IPC::Run
Hello,
Currently, the psql's test of query cancelling (src/bin/psql/t/020_cancel.pl)
gets the PPID of a running psql by using "\!" meta command, and sends SIGINT to
the process by using "kill". However, IPC::Run provides signal() routine that
sends a signal to a running process, so I think we can rewrite the test using
this routine to more simple fashion as attached patch.
What do you think about it?
Use of signal() is suggested by Fabien COELHO during the discussion of
query cancelling of pgbench [1].
[1]
https://www.postgresql.org/message-id/7691ade8-78-dd4-e26-135ccfbf0e9%40mines-paristech.fr
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
index 0765d82b92..081a1468d9 100644
--- a/src/bin/psql/t/020_cancel.pl
+++ b/src/bin/psql/t/020_cancel.pl
@@ -29,35 +29,10 @@ SKIP:
my ($stdin, $stdout, $stderr);
- # Test whether shell supports $PPID. It's part of POSIX, but some
- # pre-/non-POSIX shells don't support it (e.g., NetBSD).
- $stdin = "\\! echo \$PPID";
- IPC::Run::run([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
- '<', \$stdin, '>', \$stdout, '2>', \$stderr);
- $stdout =~ /^\d+$/ or skip "shell apparently does not support \$PPID", 2;
-
# Now start the real test
my $h = IPC::Run::start([ 'psql', '-X', '-v', 'ON_ERROR_STOP=1' ],
\$stdin, \$stdout, \$stderr);
- # Get the PID
- $stdout = '';
- $stderr = '';
- $stdin = "\\! echo \$PPID >$tempdir/psql.pid\n";
- pump $h while length $stdin;
- my $count;
- my $psql_pid;
- until (
- -s "$tempdir/psql.pid"
- and ($psql_pid =
- PostgreSQL::Test::Utils::slurp_file("$tempdir/psql.pid")) =~
- /^\d+\n/s)
- {
- ($count++ < 100 * $PostgreSQL::Test::Utils::timeout_default)
- or die "pid file did not appear";
- usleep(10_000);
- }
-
# Send sleep command and wait until the server has registered it
$stdin = "select pg_sleep($PostgreSQL::Test::Utils::timeout_default);\n";
pump $h while length $stdin;
@@ -66,7 +41,7 @@ SKIP:
) or die "timed out";
# Send cancel request
- kill 'INT', $psql_pid;
+ $h->signal('INT');
my $result = finish $h;
Re: pgbnech: allow to cancel queries during benchmark
On Wed, 9 Aug 2023 11:18:43 +0200 (CEST)
Fabien COELHO wrote:
>
> I forgot, about the test:
>
> I think that it should be integrated in the existing
> "001_pgbench_with_server.pl" script, because a TAP script is pretty
> expensive as it creates a cluster, starts it… before running the test.
Ok. I'll integrate the test into 001.
> I'm surprise that IPC::Run does not give any access to the process number.
> Anyway, its object interface seems to allow sending signal:
>
> $h->signal("...")
>
> So the code could be simplified to use that after a small delay.
Thank you for your information.
I didn't know $h->signal() and I mimicked the way of
src/bin/psql/t/020_cancel.pl to send SIGINT to a running program. I don't
know why the psql test doesn't use the interface, I'll investigate whether
this can be used in our purpose, anyway.
Regards,
Yugo Nagata
>
> --
> Fabien.
--
Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
Hello Fabien, On Wed, 9 Aug 2023 11:06:24 +0200 (CEST) Fabien COELHO wrote: > > Hello Yugo-san, > > Some feedback about v2. > > There is some dead code (&& false) which should be removed. I forgot to remove the debug code. I'll remove it. > >>> In multi-threads, only one thread can catches the signal and other threads > >>> continue to run. > > Yep. This why I see plenty uncontrolled race conditions if thread 0 > cancels clients which are managed in parallel by other threads and may be > active. I'm not really motivated to delve into libpq internals to check > whether there are possibly bad issues or not, but if two threads write > message at the same time in the same socket, I assume that this can be > bad if you are unlucky. > > ISTM that the rational convention should be that each thread cancels its > own clients, which ensures that there is no bad interaction between > threads. Actually, thread #0 and other threads never write message at the same time in the same socket. When thread #0 sends cancel requests, they are not sent to sockets that other threads are reading or writing. Rather, new another socket for cancel is created for each client, and the backend PID and cancel request are sent to the socket. PostgreSQL establishes a new connection for the cancel request, and sent a cancel signal to the specified backend. Therefore, thread #0 and other threads don't access any resources in the same time except to CancelRequested. Is still there any concern about race condition? > >>> Therefore, if Ctrl+C is pressed while threads are waiting > >>> responses from the backend in wait_on_socket_set, only one thread can be > >>> interrupted and return, but other threads will continue to wait and cannot > >>> check CancelRequested. > > >>> So, for implementing your suggestion, we need any hack > >>> to make all threads return from wait_on_socket_set when the event occurs, > >>> but > >>> I don't have idea to do this in simpler way. > > >>> In my patch, all threads can return from wait_on_socket_set at Ctrl+C > >>> because when thread #0 cancels all connections, the following error is > >>> sent to all sessions: > >>> > >>> ERROR: canceling statement due to user request > >>> > >>> and all threads will receive the response from the backend. > > Hmmm. > > I understand that the underlying issue you are raising is that other > threads may be stuck while waiting on socket events and that with your > approach they will be cleared somehow by socket 0. > > I'll say that (1) this point does not address potential race condition > issues with several thread interacting directly on the same client ; > (2) thread 0 may also be stuck waiting for events so the cancel is only > taken into account when it is woken up. I answered to (1) the consern about race condition above. And, as to (2), the SIGINT signal is handle only in thread #0 because it is blocked in other threads. So, when SIGINT is delivered, thread #0 will be interrupted and woken up immediately from waiting on socket, returning EINTR. Therefore, thread #0 would not be stuck. Regards, Yugo Nagata > If we accept that each thread cancels its clients when it is woken up, > which may imply some (usually small) delay, then it is not so different > from the current version because the code must wait for 0 to wake up > anyway, and it solves (1). The current version does not address potential > thread interactions. > > -- > Fabien. -- Yugo NAGATA
Re: pgbench: allow to exit immediately when any client is aborted
On Wed, 9 Aug 2023 13:59:21 +0900
Yugo NAGATA wrote:
> On Wed, 9 Aug 2023 10:46:38 +0900
> Yugo NAGATA wrote:
>
> > On Wed, 9 Aug 2023 02:15:01 +0200 (CEST)
> > Fabien COELHO wrote:
> >
> > >
> > > Hello Yugo-san,
> > >
> > > > There are cases where "goto done" is used where some error like
> > > > "invalid socket: ..." happens. I would like to make pgbench exit in
> > > > such cases, too, so I chose to handle errors below the "done:" label.
> > > > However, we can change it to call "exit" instead of "goo done" at each
> > > > place. Do you think this is better?
> > >
> > > Good point.
> > >
> > > Now I understand the "!= FINISHED", because indeed in these cases the
> > > done
> > > is reached with unfinished but not necessarily ABORTED clients, and the
> > > comment was somehow misleading.
> > >
> > > On reflection, there should be only one exit() call, thus I'd say to keep
> > > the "goto done" as you did, but to move the checking loop *before* the
> > > disconnect_all, and the overall section comment could be something like
> > > "possibly abort if any client is not finished, meaning some error
> > > occured", which is consistent with the "!= FINISHED" condition.
> >
> > Thank you for your suggestion.
> > I'll fix as above and submit a updated patch soon.
>
> I attached the updated patch v3 including changes above, a test,
> and fix of the typo you pointed out.
I'm sorry but the test in the previous patch was incorrect.
I attached the correct one.
Regards,
Yugo Nagata
> Regards,
> Yugo Nagata
>
> >
> > Regards,
> > Yugo Nagata
> >
> > --
> > Yugo NAGATA
> >
> >
>
>
> --
> Yugo NAGATA
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 6c5c8afa6d..83fe01c33f 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -787,6 +787,19 @@ pgbench options d
+
+ --exit-on-abort
+
+
+Exit immediately when any client is aborted due to some error. Without
+this option, even when a client is aborted, other clients could continue
+their run as specified by -t or -T option,
+and pgbench will print an incomplete results
+in this case.
+
+
+
+
--log-prefix=prefix
@@ -985,7 +998,8 @@ pgbench options d
benchmark such as initial connection failures also exit with status 1.
Errors during the run such as database errors or problems in the script
will result in exit status 2. In the latter case,
- pgbench will print partial results.
+ pgbench will print partial results if
+ --exit-on-abort option is not specified.
@@ -2801,14 +2815,17 @@ statement latencies in milliseconds, failures and retries:
start a connection to the database server / the socket for connecting
the client to the database server has become invalid). In such cases
all clients of this thread stop while other threads continue to work.
+ However, --exit-on-abort is specified, all of the
+ threads stop immediately in this case.
Direct client errors. They lead to immediate exit from
pgbench with the corresponding error message
- only in the case of an internal pgbench
- error (which are supposed to never occur...). Otherwise in the worst
+ in the case of an internal pgbench
+ error (which are supposed to never occur...) or when
+ --exit-on-abort is specified . Otherwise in the worst
case they only lead to the abortion of the failed client while other
clients continue their run (but some client errors are handled without
an abortion of the client and reported separately, see below). Later in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..eca55d0230 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -765,6 +765,8 @@ static int64 total_weight = 0;
static bool verbose_errors = false; /* print verbose messages of all errors */
+static bool exit_on_abort = false; /* exit when any client is aborted */
+
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -911,6 +913,7 @@ usage(void)
" -T, --time=NUM duration of benchmark test in seconds\n"
" -v, --vacuum-all vacuum all four standard tables before tests\n"
" --aggregate-interval=NUM aggregate data over NUM seconds\n"
+ " --exit-on-abort
Re: pgbench: allow to exit immediately when any client is aborted
On Wed, 9 Aug 2023 10:46:38 +0900
Yugo NAGATA wrote:
> On Wed, 9 Aug 2023 02:15:01 +0200 (CEST)
> Fabien COELHO wrote:
>
> >
> > Hello Yugo-san,
> >
> > > There are cases where "goto done" is used where some error like
> > > "invalid socket: ..." happens. I would like to make pgbench exit in
> > > such cases, too, so I chose to handle errors below the "done:" label.
> > > However, we can change it to call "exit" instead of "goo done" at each
> > > place. Do you think this is better?
> >
> > Good point.
> >
> > Now I understand the "!= FINISHED", because indeed in these cases the done
> > is reached with unfinished but not necessarily ABORTED clients, and the
> > comment was somehow misleading.
> >
> > On reflection, there should be only one exit() call, thus I'd say to keep
> > the "goto done" as you did, but to move the checking loop *before* the
> > disconnect_all, and the overall section comment could be something like
> > "possibly abort if any client is not finished, meaning some error
> > occured", which is consistent with the "!= FINISHED" condition.
>
> Thank you for your suggestion.
> I'll fix as above and submit a updated patch soon.
I attached the updated patch v3 including changes above, a test,
and fix of the typo you pointed out.
Regards,
Yugo Nagata
>
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 6c5c8afa6d..83fe01c33f 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -787,6 +787,19 @@ pgbench options d
+
+ --exit-on-abort
+
+
+Exit immediately when any client is aborted due to some error. Without
+this option, even when a client is aborted, other clients could continue
+their run as specified by -t or -T option,
+and pgbench will print an incomplete results
+in this case.
+
+
+
+
--log-prefix=prefix
@@ -985,7 +998,8 @@ pgbench options d
benchmark such as initial connection failures also exit with status 1.
Errors during the run such as database errors or problems in the script
will result in exit status 2. In the latter case,
- pgbench will print partial results.
+ pgbench will print partial results if
+ --exit-on-abort option is not specified.
@@ -2801,14 +2815,17 @@ statement latencies in milliseconds, failures and retries:
start a connection to the database server / the socket for connecting
the client to the database server has become invalid). In such cases
all clients of this thread stop while other threads continue to work.
+ However, --exit-on-abort is specified, all of the
+ threads stop immediately in this case.
Direct client errors. They lead to immediate exit from
pgbench with the corresponding error message
- only in the case of an internal pgbench
- error (which are supposed to never occur...). Otherwise in the worst
+ in the case of an internal pgbench
+ error (which are supposed to never occur...) or when
+ --exit-on-abort is specified . Otherwise in the worst
case they only lead to the abortion of the failed client while other
clients continue their run (but some client errors are handled without
an abortion of the client and reported separately, see below). Later in
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..eca55d0230 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -765,6 +765,8 @@ static int64 total_weight = 0;
static bool verbose_errors = false; /* print verbose messages of all errors */
+static bool exit_on_abort = false; /* exit when any client is aborted */
+
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -911,6 +913,7 @@ usage(void)
" -T, --time=NUM duration of benchmark test in seconds\n"
" -v, --vacuum-all vacuum all four standard tables before tests\n"
" --aggregate-interval=NUM aggregate data over NUM seconds\n"
+ " --exit-on-abort exit when any client is aborted\n"
" --failures-detailed report the failures grouped by basic types\n"
" --log-prefix=PREFIX prefix for transaction time log file\n"
" (default: \"pgbench_log\")\n"
@@ -6612,6 +6615,7 @@ main(int argc, char **argv)
{"failures-detailed", no_argument, NULL, 13},
{&quo
Re: pgbench: allow to exit immediately when any client is aborted
On Wed, 9 Aug 2023 02:15:01 +0200 (CEST) Fabien COELHO wrote: > > Hello Yugo-san, > > > There are cases where "goto done" is used where some error like > > "invalid socket: ..." happens. I would like to make pgbench exit in > > such cases, too, so I chose to handle errors below the "done:" label. > > However, we can change it to call "exit" instead of "goo done" at each > > place. Do you think this is better? > > Good point. > > Now I understand the "!= FINISHED", because indeed in these cases the done > is reached with unfinished but not necessarily ABORTED clients, and the > comment was somehow misleading. > > On reflection, there should be only one exit() call, thus I'd say to keep > the "goto done" as you did, but to move the checking loop *before* the > disconnect_all, and the overall section comment could be something like > "possibly abort if any client is not finished, meaning some error > occured", which is consistent with the "!= FINISHED" condition. Thank you for your suggestion. I'll fix as above and submit a updated patch soon. Regards, Yugo Nagata -- Yugo NAGATA
Re: pgbench: allow to exit immediately when any client is aborted
Hello Fabien, On Mon, 7 Aug 2023 12:17:38 +0200 (CEST) Fabien COELHO wrote: > > Hello Yugo-san, > > > I attached v2 patch including the documentation and some comments > > in the code. > > I've looked at this patch. Thank you for your review! > > I'm unclear whether it does what it says: "exit immediately on abort", I > would expect a cold call to "exit" (with a clear message obviously) when > the abort occurs. > > Currently it skips to "done" which starts by closing this particular > thread client connections, then it will call "exit" later, so it is not > "immediate". There are cases where "goto done" is used where some error like "invalid socket: ..." happens. I would like to make pgbench exit in such cases, too, so I chose to handle errors below the "done:" label. However, we can change it to call "exit" instead of "goo done" at each place. Do you think this is better? > I do not think that this cleanup is necessary, because anyway all other > threads will be brutally killed by the exit called by the aborted thread, > so why bothering to disconnect only some connections? Agreed. This disconnection is not necessary anyway even when we would like to handle it below "done". > /* If any client is aborted, exit immediately. */ > if (state[i].state != CSTATE_FINISHED) > > For this comment, I would prefer "if ( ... == CSTATE_ABORTED)" rather that > implying that not finished means aborted, and if you follow my previous > remark then this code can be removed. Ok. If we handle errors like "invalid socket:" (mentioned above) after skipping to "done", we should set the status to CSTATE_ABORTED before the jump. Otherwise, if we choose to call "exit" immediately at each error instead of skipping to "done", we can remove this as you says. > Typo: "going to exist" -> "going to exit". Note that it is not "the whole > thread" but "the program" which is exiting. I'll fix. > There is no test. I'll add an test. Regards, Yugo Nagata > -- > Fabien. -- Yugo NAGATA
Re: pgbench: allow to exit immediately when any client is aborted
On Mon, 7 Aug 2023 11:02:48 +0900 Yugo NAGATA wrote: > On Sat, 05 Aug 2023 12:16:11 +0900 (JST) > Tatsuo Ishii wrote: > > > > Hi, > > > > > > I would like to propose to add an option to pgbench so that benchmark > > > can quit immediately when any client is aborted. Currently, when a > > > client is aborted due to some error, for example, network trouble, > > > other clients continue their run until a certain number of transactions > > > specified -t is reached or the time specified by -T is expired. At the > > > end, the results are printed, but they are not useful, as the message > > > "Run was aborted; the above results are incomplete" shows. > > > > Sounds like a good idea. It's a waste of resources waiting for > > unusable benchmark results until t/T expired. If we graze on the > > screen, then it's easy to cancel the pgbench run. But I frequently let > > pgbench run without sitting in front of the screen especially when t/T > > is large (it's recommended that running pgbench with large enough t/T > > to get usable results). > > Thank you for your agreement. > > > > For precise benchmark purpose, we would not want to wait to get such > > > incomplete results, rather we would like to know some trouble happened > > > to allow a quick retry. Therefore, it would be nice to add an option to > > > make pgbench exit instead of continuing run in other clients when any > > > client is aborted. I think adding the optional is better than whole > > > behavioural change because some users that use pgbench just in order > > > to stress on backends for testing purpose rather than benchmark might > > > not want to stop pgbench even a client is aborted. > > > > > > Attached is the patch to add the option --exit-on-abort. > > > If this option is specified, when any client is aborted, pgbench > > > immediately quit by calling exit(2). I attached v2 patch including the documentation and some comments in the code. Regards, Yugo Nagata > > > > > > What do you think about it? > > > > I think aborting pgbench by calling exit(2) is enough. It's not worth > > the trouble to add more codes for this purpose. > > In order to stop pgbench more gracefully, it might be better to make > each thread exit at more proper timing after some cleaning-up like > connection close. However, pgbench code doesn't provide such functions > for inter-threads communication. If we would try to make this, both > pthread and Windows versions would be needed. I don't think it is necessary > to make such effort for --exit-on-abort option, as you said. > > Regards, > Yugo Nagata > > > > > Best reagards, > > -- > > Tatsuo Ishii > > SRA OSS LLC > > English: http://www.sraoss.co.jp/index_en/ > > Japanese:http://www.sraoss.co.jp > > > -- > Yugo NAGATA > > -- Yugo NAGATA diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 6c5c8afa6d..83fe01c33f 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -787,6 +787,19 @@ pgbench options d + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + + + --log-prefix=prefix @@ -985,7 +998,8 @@ pgbench options d benchmark such as initial connection failures also exit with status 1. Errors during the run such as database errors or problems in the script will result in exit status 2. In the latter case, - pgbench will print partial results. + pgbench will print partial results if + --exit-on-abort option is not specified. @@ -2801,14 +2815,17 @@ statement latencies in milliseconds, failures and retries: start a connection to the database server / the socket for connecting the client to the database server has become invalid). In such cases all clients of this thread stop while other threads continue to work. + However, --exit-on-abort is specified, all of the + threads stop immediately in this case. Direct client errors. They lead to immediate exit from pgbench with the corresponding error message - only in the case of an internal pgbench - error (which are supposed to never occur...). Otherwise in the worst + in the case of an internal pgbench + error (which ar
Re: pgbench: allow to exit immediately when any client is aborted
On Sat, 05 Aug 2023 12:16:11 +0900 (JST) Tatsuo Ishii wrote: > > Hi, > > > > I would like to propose to add an option to pgbench so that benchmark > > can quit immediately when any client is aborted. Currently, when a > > client is aborted due to some error, for example, network trouble, > > other clients continue their run until a certain number of transactions > > specified -t is reached or the time specified by -T is expired. At the > > end, the results are printed, but they are not useful, as the message > > "Run was aborted; the above results are incomplete" shows. > > Sounds like a good idea. It's a waste of resources waiting for > unusable benchmark results until t/T expired. If we graze on the > screen, then it's easy to cancel the pgbench run. But I frequently let > pgbench run without sitting in front of the screen especially when t/T > is large (it's recommended that running pgbench with large enough t/T > to get usable results). Thank you for your agreement. > > For precise benchmark purpose, we would not want to wait to get such > > incomplete results, rather we would like to know some trouble happened > > to allow a quick retry. Therefore, it would be nice to add an option to > > make pgbench exit instead of continuing run in other clients when any > > client is aborted. I think adding the optional is better than whole > > behavioural change because some users that use pgbench just in order > > to stress on backends for testing purpose rather than benchmark might > > not want to stop pgbench even a client is aborted. > > > > Attached is the patch to add the option --exit-on-abort. > > If this option is specified, when any client is aborted, pgbench > > immediately quit by calling exit(2). > > > > What do you think about it? > > I think aborting pgbench by calling exit(2) is enough. It's not worth > the trouble to add more codes for this purpose. In order to stop pgbench more gracefully, it might be better to make each thread exit at more proper timing after some cleaning-up like connection close. However, pgbench code doesn't provide such functions for inter-threads communication. If we would try to make this, both pthread and Windows versions would be needed. I don't think it is necessary to make such effort for --exit-on-abort option, as you said. Regards, Yugo Nagata > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA
pgbench: allow to exit immediately when any client is aborted
Hi,
I would like to propose to add an option to pgbench so that benchmark
can quit immediately when any client is aborted. Currently, when a
client is aborted due to some error, for example, network trouble,
other clients continue their run until a certain number of transactions
specified -t is reached or the time specified by -T is expired. At the
end, the results are printed, but they are not useful, as the message
"Run was aborted; the above results are incomplete" shows.
For precise benchmark purpose, we would not want to wait to get such
incomplete results, rather we would like to know some trouble happened
to allow a quick retry. Therefore, it would be nice to add an option to
make pgbench exit instead of continuing run in other clients when any
client is aborted. I think adding the optional is better than whole
behavioural change because some users that use pgbench just in order
to stress on backends for testing purpose rather than benchmark might
not want to stop pgbench even a client is aborted.
Attached is the patch to add the option --exit-on-abort.
If this option is specified, when any client is aborted, pgbench
immediately quit by calling exit(2).
What do you think about it?
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..6fae5a43e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -765,6 +765,8 @@ static int64 total_weight = 0;
static bool verbose_errors = false; /* print verbose messages of all errors */
+static bool exit_on_abort = false; /* exit when any client is aborted */
+
/* Builtin test scripts */
typedef struct BuiltinScript
{
@@ -911,6 +913,7 @@ usage(void)
" -T, --time=NUM duration of benchmark test in seconds\n"
" -v, --vacuum-all vacuum all four standard tables before tests\n"
" --aggregate-interval=NUM aggregate data over NUM seconds\n"
+ " --exit-on-abort exit when any client is aborted\n"
" --failures-detailed report the failures grouped by basic types\n"
" --log-prefix=PREFIX prefix for transaction time log file\n"
" (default: \"pgbench_log\")\n"
@@ -6612,6 +6615,7 @@ main(int argc, char **argv)
{"failures-detailed", no_argument, NULL, 13},
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
+ {"exit-on-abort", no_argument, NULL, 16},
{NULL, 0, NULL, 0}
};
@@ -6945,6 +6949,10 @@ main(int argc, char **argv)
benchmarking_option_set = true;
verbose_errors = true;
break;
+ case 16: /* exit-on-abort */
+benchmarking_option_set = true;
+exit_on_abort = true;
+break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7553,11 +7561,13 @@ threadRun(void *arg)
advanceConnectionState(thread, st, );
+ if (exit_on_abort && st->state == CSTATE_ABORTED)
+goto done;
/*
* If advanceConnectionState changed client to finished state,
* that's one fewer client that remains.
*/
- if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
+ else if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
remains--;
}
@@ -7592,6 +7602,15 @@ threadRun(void *arg)
done:
disconnect_all(state, nstate);
+ for (int i = 0; i < nstate; i++)
+ {
+ if (state[i].state != CSTATE_FINISHED)
+ {
+ pg_log_error("Run was aborted due to an error in thread %d", thread->tid);
+ exit(2);
+ }
+ }
+
if (thread->logfile)
{
if (agg_interval > 0)
Re: pgbnech: allow to cancel queries during benchmark
On Wed, 2 Aug 2023 16:37:53 +0900
Yugo NAGATA wrote:
> Hello Fabien,
>
> On Fri, 14 Jul 2023 20:32:01 +0900
> Yugo NAGATA wrote:
>
> I attached the updated patch.
I'm sorry. I forgot to attach the patch.
Regards,
Yugo Nagata
>
> > Hello Fabien,
> >
> > Thank you for your review!
> >
> > On Mon, 3 Jul 2023 20:39:23 +0200 (CEST)
> > Fabien COELHO wrote:
> >
> > >
> > > Yugo-san,
> > >
> > > Some feedback about v1 of this patch.
> > >
> > > Patch applies cleanly, compiles.
> > >
> > > There are no tests, could there be one? ISTM that one could be done with
> > > a
> > > "SELECT pg_sleep(...)" script??
> >
> > Agreed. I will add the test.
>
> I added a TAP test.
>
> >
> > > The global name "all_state" is quite ambiguous, what about
> > > "client_states"
> > > instead? Or maybe it could be avoided, see below.
> > >
> > > Instead of renaming "state" to "all_state" (or client_states as suggested
> > > above), I'd suggest to minimize the patch by letting "state" inside the
> > > main and adding a "client_states = state;" just after the allocation, or
> > > another approach, see below.
> >
> > Ok, I'll fix to add a global variable "client_states" and make this point to
> > "state" instead of changing "state" to global.
>
> Done.
>
> >
> > > Should PQfreeCancel be called on deconnections, in finishCon? I think
> > > that
> > > there may be a memory leak with the current implementation??
> >
> > Agreed. I'll fix.
>
> Done.
>
> Regards,
> Yugo Nagata
>
> >
> > > Maybe it should check that cancel is not NULL before calling PQcancel?
> >
> > I think this is already checked as below, but am I missing something?
> >
> > + if (all_state[i].cancel != NULL)
> > + (void) PQcancel(all_state[i].cancel, errbuf, sizeof(errbuf));
> >
> > > After looking at the code, I'm very unclear whether they may be some
> > > underlying race conditions, or not, depending on when the cancel is
> > > triggered. I think that some race conditions are still likely given the
> > > current thread 0 implementation, and dealing with them with a barrier or
> > > whatever is not desirable at all.
> > >
> > > In order to work around this issue, ISTM that we should go away from the
> > > simple and straightforward thread 0 approach, and the only clean way is
> > > that the cancelation should be managed by each thread for its own client.
> > >
> > > I'd suggest to have the advanceState to call PQcancel when
> > > CancelRequested
> > > is set and switch to CSTATE_ABORTED to end properly. This means that
> > > there
> > > would be no need for the global client states pointer, so the patch
> > > should
> > > be smaller and simpler. Possibly there would be some shortcuts added here
> > > and there to avoid lingering after the control-C, in threadRun.
> >
> > I am not sure this approach is simpler than mine.
> >
> > In multi-threads, only one thread can catches the signal and other threads
> > continue to run. Therefore, if Ctrl+C is pressed while threads are waiting
> > responses from the backend in wait_on_socket_set, only one thread can be
> > interrupted and return, but other threads will continue to wait and cannot
> > check CancelRequested. So, for implementing your suggestion, we need any
> > hack
> > to make all threads return from wait_on_socket_set when the event occurs,
> > but
> > I don't have idea to do this in simpler way.
> >
> > In my patch, all threads can return from wait_on_socket_set at Ctrl+C
> > because when thread #0 cancels all connections, the following error is
> > sent to all sessions:
> >
> > ERROR: canceling statement due to user request
> >
> > and all threads will receive the response from the backend.
> >
> > Regards,
> > Yugo Nagata
> >
> > --
> > Yugo NAGATA
> >
> >
>
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 539c2795e2..68278d2b18 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* que
Re: pgbnech: allow to cancel queries during benchmark
Hello Fabien, On Fri, 14 Jul 2023 20:32:01 +0900 Yugo NAGATA wrote: I attached the updated patch. > Hello Fabien, > > Thank you for your review! > > On Mon, 3 Jul 2023 20:39:23 +0200 (CEST) > Fabien COELHO wrote: > > > > > Yugo-san, > > > > Some feedback about v1 of this patch. > > > > Patch applies cleanly, compiles. > > > > There are no tests, could there be one? ISTM that one could be done with a > > "SELECT pg_sleep(...)" script?? > > Agreed. I will add the test. I added a TAP test. > > > The global name "all_state" is quite ambiguous, what about "client_states" > > instead? Or maybe it could be avoided, see below. > > > > Instead of renaming "state" to "all_state" (or client_states as suggested > > above), I'd suggest to minimize the patch by letting "state" inside the > > main and adding a "client_states = state;" just after the allocation, or > > another approach, see below. > > Ok, I'll fix to add a global variable "client_states" and make this point to > "state" instead of changing "state" to global. Done. > > > Should PQfreeCancel be called on deconnections, in finishCon? I think that > > there may be a memory leak with the current implementation?? > > Agreed. I'll fix. Done. Regards, Yugo Nagata > > > Maybe it should check that cancel is not NULL before calling PQcancel? > > I think this is already checked as below, but am I missing something? > > + if (all_state[i].cancel != NULL) > + (void) PQcancel(all_state[i].cancel, errbuf, sizeof(errbuf)); > > > After looking at the code, I'm very unclear whether they may be some > > underlying race conditions, or not, depending on when the cancel is > > triggered. I think that some race conditions are still likely given the > > current thread 0 implementation, and dealing with them with a barrier or > > whatever is not desirable at all. > > > > In order to work around this issue, ISTM that we should go away from the > > simple and straightforward thread 0 approach, and the only clean way is > > that the cancelation should be managed by each thread for its own client. > > > > I'd suggest to have the advanceState to call PQcancel when CancelRequested > > is set and switch to CSTATE_ABORTED to end properly. This means that there > > would be no need for the global client states pointer, so the patch should > > be smaller and simpler. Possibly there would be some shortcuts added here > > and there to avoid lingering after the control-C, in threadRun. > > I am not sure this approach is simpler than mine. > > In multi-threads, only one thread can catches the signal and other threads > continue to run. Therefore, if Ctrl+C is pressed while threads are waiting > responses from the backend in wait_on_socket_set, only one thread can be > interrupted and return, but other threads will continue to wait and cannot > check CancelRequested. So, for implementing your suggestion, we need any hack > to make all threads return from wait_on_socket_set when the event occurs, but > I don't have idea to do this in simpler way. > > In my patch, all threads can return from wait_on_socket_set at Ctrl+C > because when thread #0 cancels all connections, the following error is > sent to all sessions: > > ERROR: canceling statement due to user request > > and all threads will receive the response from the backend. > > Regards, > Yugo Nagata > > -- > Yugo NAGATA > > -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
Hello Fabien, Thank you for your review! On Mon, 3 Jul 2023 20:39:23 +0200 (CEST) Fabien COELHO wrote: > > Yugo-san, > > Some feedback about v1 of this patch. > > Patch applies cleanly, compiles. > > There are no tests, could there be one? ISTM that one could be done with a > "SELECT pg_sleep(...)" script?? Agreed. I will add the test. > The global name "all_state" is quite ambiguous, what about "client_states" > instead? Or maybe it could be avoided, see below. > > Instead of renaming "state" to "all_state" (or client_states as suggested > above), I'd suggest to minimize the patch by letting "state" inside the > main and adding a "client_states = state;" just after the allocation, or > another approach, see below. Ok, I'll fix to add a global variable "client_states" and make this point to "state" instead of changing "state" to global. > Should PQfreeCancel be called on deconnections, in finishCon? I think that > there may be a memory leak with the current implementation?? Agreed. I'll fix. > Maybe it should check that cancel is not NULL before calling PQcancel? I think this is already checked as below, but am I missing something? + if (all_state[i].cancel != NULL) + (void) PQcancel(all_state[i].cancel, errbuf, sizeof(errbuf)); > After looking at the code, I'm very unclear whether they may be some > underlying race conditions, or not, depending on when the cancel is > triggered. I think that some race conditions are still likely given the > current thread 0 implementation, and dealing with them with a barrier or > whatever is not desirable at all. > > In order to work around this issue, ISTM that we should go away from the > simple and straightforward thread 0 approach, and the only clean way is > that the cancelation should be managed by each thread for its own client. > > I'd suggest to have the advanceState to call PQcancel when CancelRequested > is set and switch to CSTATE_ABORTED to end properly. This means that there > would be no need for the global client states pointer, so the patch should > be smaller and simpler. Possibly there would be some shortcuts added here > and there to avoid lingering after the control-C, in threadRun. I am not sure this approach is simpler than mine. In multi-threads, only one thread can catches the signal and other threads continue to run. Therefore, if Ctrl+C is pressed while threads are waiting responses from the backend in wait_on_socket_set, only one thread can be interrupted and return, but other threads will continue to wait and cannot check CancelRequested. So, for implementing your suggestion, we need any hack to make all threads return from wait_on_socket_set when the event occurs, but I don't have idea to do this in simpler way. In my patch, all threads can return from wait_on_socket_set at Ctrl+C because when thread #0 cancels all connections, the following error is sent to all sessions: ERROR: canceling statement due to user request and all threads will receive the response from the backend. Regards, Yugo Nagata -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Fri, 7 Jul 2023 17:21:36 +0900
Yugo NAGATA wrote:
> Hi Nikita,
>
> On Wed, 5 Jul 2023 17:49:20 +0300
> Nikita Malakhov wrote:
>
> > Hi!
> >
> > I like the idea of having a standard function which shows a TOAST value ID
> > for a row. I've used my own to handle TOAST errors. Just, maybe, more
> > correct
> > name would be "...value_id", because you actually retrieve valueid field
> > from the TOAST pointer, and chunk ID consists of valueid + chunk_seq.
>
> Thank you for your review!
>
> Although, the retrieved field is "va_valueid" and it is called "value ID" in
> the
> code, I chose the name "..._chunk_id" because I found the description in the
> documentation as followings:
>
> -
> Every TOAST table has the columns chunk_id (an OID identifying the particular
> TOASTed value), chunk_seq (a sequence number for the chunk within its value),
> and chunk_data (the actual data of the chunk). A unique index on chunk_id and
> chunk_seq provides fast retrieval of the values. A pointer datum representing
> an out-of-line on-disk TOASTed value therefore needs to store the OID of the
> TOAST table in which to look and the OID of the specific value (its chunk_id)
> -
> https://www.postgresql.org/docs/devel/storage-toast.html
>
> Here, chunk_id defined separately from chunk_seq. Therefore, I wonder
> pg_column_toast_chunk_id would be ok. However, I don't insist on this
> and I would be happy to change it if the other name is more natural for users.
I attached v2 patch that contains the documentation fix.
Regards,
Yugo Nagata
>
> Regards,
> Yugo Nagata
>
> >
> > --
> > Regards,
> > Nikita Malakhov
> > Postgres Professional
> > The Russian Postgres Company
> > https://postgrespro.ru/
>
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5a47ce4343..c2c3156cd4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27608,6 +27608,20 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Returns the chunk id of the TOASTed value, or NULL
+if the value is not TOASTed.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 884bfbc8ce..fe8788c1b1 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5069,6 +5069,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk id of the TOASTed value.
+ * Return NULL for unTOASTed value.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 6996073989..0cacd0391d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7403,6 +7403,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of TOASTed value',
+ proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype => 'oid',
+ proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
{ oid => '2322',
descr => 'total disk space usage for the specified tablespace',
proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
Hi Nikita, On Wed, 5 Jul 2023 17:49:20 +0300 Nikita Malakhov wrote: > Hi! > > I like the idea of having a standard function which shows a TOAST value ID > for a row. I've used my own to handle TOAST errors. Just, maybe, more > correct > name would be "...value_id", because you actually retrieve valueid field > from the TOAST pointer, and chunk ID consists of valueid + chunk_seq. Thank you for your review! Although, the retrieved field is "va_valueid" and it is called "value ID" in the code, I chose the name "..._chunk_id" because I found the description in the documentation as followings: - Every TOAST table has the columns chunk_id (an OID identifying the particular TOASTed value), chunk_seq (a sequence number for the chunk within its value), and chunk_data (the actual data of the chunk). A unique index on chunk_id and chunk_seq provides fast retrieval of the values. A pointer datum representing an out-of-line on-disk TOASTed value therefore needs to store the OID of the TOAST table in which to look and the OID of the specific value (its chunk_id) - https://www.postgresql.org/docs/devel/storage-toast.html Here, chunk_id defined separately from chunk_seq. Therefore, I wonder pg_column_toast_chunk_id would be ok. However, I don't insist on this and I would be happy to change it if the other name is more natural for users. Regards, Yugo Nagata > > -- > Regards, > Nikita Malakhov > Postgres Professional > The Russian Postgres Company > https://postgrespro.ru/ -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Wed, 28 Jun 2023 00:01:02 +0800 jian he wrote: > On Thu, Jun 1, 2023 at 2:47 AM Yugo NAGATA wrote: > > > > On Thu, 1 Jun 2023 23:59:09 +0900 > > Yugo NAGATA wrote: > > > > > Hello hackers, > > > > > > Here's a rebased version of the patch-set adding Incremental View > > > Maintenance support for PostgreSQL. That was discussed in [1]. > > > > > [1] > > > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp > > > > --- > > * Overview > > > > Incremental View Maintenance (IVM) is a way to make materialized views > > up-to-date by computing only incremental changes and applying them on > > views. IVM is more efficient than REFRESH MATERIALIZED VIEW when > > only small parts of the view are changed. > > > > ** Feature > > > > The attached patchset provides a feature that allows materialized views > > to be updated automatically and incrementally just after a underlying > > table is modified. > > > > You can create an incementally maintainable materialized view (IMMV) > > by using CREATE INCREMENTAL MATERIALIZED VIEW command. > > > > The followings are supported in view definition queries: > > - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) > > - some built-in aggregate functions (count, sum, avg, min, max) > > - GROUP BY clause > > - DISTINCT clause > > > > Views can contain multiple tuples with the same content (duplicate tuples). > > > > ** Restriction > > > > The following are not supported in a view definition: > > - Outer joins > > - Aggregates otehr than above, window functions, HAVING > > - Sub-queries, CTEs > > - Set operations (UNION, INTERSECT, EXCEPT) > > - DISTINCT ON, ORDER BY, LIMIT, OFFSET > > > > Also, a view definition query cannot contain other views, materialized > > views, > > foreign tables, partitioned tables, partitions, VALUES, non-immutable > > functions, > > system columns, or expressions that contains aggregates. > > > > --- > > * Design > > > > An IMMV is maintained using statement-level AFTER triggers. > > When an IMMV is created, triggers are automatically created on all base > > tables contained in the view definition query. > > > > When a table is modified, changes that occurred in the table are extracted > > as transition tables in the AFTER triggers. Then, changes that will occur in > > the view are calculated by a rewritten view dequery in which the modified > > table > > is replaced with the transition table. > > > > For example, if the view is defined as "SELECT * FROM R, S", and tuples > > inserted > > into R are stored in a transiton table dR, the tuples that will be inserted > > into > > the view are calculated as the result of "SELECT * FROM dR, S". > > > > ** Multiple Tables Modification > > > > Multiple tables can be modified in a statement when using triggers, foreign > > key > > constraint, or modifying CTEs. When multiple tables are modified, we need > > the state of tables before the modification. > > > > For example, when some tuples, dR and dS, are inserted into R and S > > respectively, > > the tuples that will be inserted into the view are calculated by the > > following > > two queries: > > > > "SELECT * FROM dR, S_pre" > > "SELECT * FROM R, dS" > > > > where S_pre is the table before the modification, R is the current state of > > table, that is, after the modification. This pre-update states of table > > is calculated by filtering inserted tuples and appending deleted tuples. > > The subquery that represents pre-update state is generated in > > get_prestate_rte(). > > Specifically, the insterted tuples are filtered by calling > > IVM_visible_in_prestate() > > in WHERE clause. This function checks the visibility of tuples by using > > the snapshot taken before table modification. The deleted tuples are > > contained > > in the old transition table, and this table is appended using UNION ALL. > > > > Transition tables for each modification are collected in each AFTER trigger > > function call. Then, the view maintenance is performed in the last call of > > the trigger. > > > > In the original PostgreSQL, tuplestores of tr
Re: Request for new function in view update
On Thu, 1 Jun 2023 12:18:47 -0500 Terry Brennan wrote: > Hello all, > > I am a researcher in databases who would like to suggest a new function. I > am writing to you because you have an active developer community. Your > website said that suggestions for new functions should go to this mailing > list. If there is another mailing list you prefer, please let me know. > > My research is in updating views -- the problem of translating an update in > a view to an update to a set of underlying base tables. This problem has > been partially solved for many years, including in PostgreSQL, but a > complete solution hasn't been found. > > Views are useful for data independence; if users only access data through > views, then underlying databases can change without user programs. Data > independence requires an automatic solution to the view update problem. > > In my research, I went back to the initial papers about the problem. The > most promising approach was the "constant complement" approach. It starts > from the idea that a view shows only part of the information in a database, > and that view updates should never change the part of the database that > isn't exposed in the view. (The "complement" is the unexposed part, and > "constant" means that a view update shouldn't change the complement.) The > "constant complement" constraint is intuitive, that a view update shouldn't > have side effects on information not available through the view. > > A seminal paper showed that defining a complement is enough, because each > complement of a view creates a unique view update. Unfortunately, there > are limitations. Views have multiple complements, and no unique minimal > complement exists. Because of this limitation and other practical > difficulties, the constant complement approach was abandoned. > > I used a theorem in this initial paper that other researchers didn't use, > that shows the inverse. An update method defines a unique complement. I > used the two theorems as a saw's upstroke and downstroke to devise view > update methods for several relational operators. Unlike other approaches, > these methods have a solid mathematical foundation. > > Some relational operators are easy (selection), others are hard > (projection); some have several valid update methods that can be used > interchangeably (union) and some can have several valid update methods that > reflect different semantics (joins). For joins, I found clues in the > database that can determine which update method to use. I address the > other relational operators, but not in the attached paper > . > I also discuss the problem of when views can't have updates, and possible > reasons why. > > I have attached my arXiv paper. I would appreciate anyone's interest in > this topic. I'm interested in the view update problem because we have some works on this topic [1][2]. I read your paper. Although I don't understand the theoretical part enough, I found your proposal methods to update views as for several relational operators. The method for updating selection views seems same as the way of automatically updatable views in the current PostgreSQL, that is, deleting/updating rows in a view results in deletes/updates for corresponding rows in the base table. Inserting rows that is not compliant to the view is checked and prevented if the view is defined with WITH CHECK OPTION. However, the proposed method for projection is that a column not contained in the view is updated to NULL when a row is deleted. I think it is not desirable to use NULL in a such special purpose, and above all, this is different the current PostgreSQL behavior, so I wonder it would not accepted to change it. I think it would be same for the method for JOIN that uses NULL for the special use. I think it would be nice to extend view updatability of PostgreSQL because the SQL standard allows more than the current limited support. In this case, I wonder we should follow SQL:1999 or later, and maybe this would be somehow compatible to the spec in Oracle. [1] https://dl.acm.org/doi/10.1145/3164541.3164584 [2] https://www.pgcon.org/2017/schedule/events/1074.en.html Regards, Yugo Nagata > Yours > Terry Brennan -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
On Mon, 26 Jun 2023 12:59:21 -0400 Kirk Wolak wrote: > On Mon, Jun 26, 2023 at 9:46 AM Yugo NAGATA wrote: > > > Hello, > > > > This attached patch enables pgbench to cancel queries during benchmark. > > > > Formerly, Ctrl+C during benchmark killed pgbench immediately, but backend > > processes executing long queries remained for a while. You can simply > > reproduce this problem by cancelling the pgbench running a custom script > > executing "SELECT pg_sleep(10)". > > > > The patch fixes this so that cancel requests are sent for all connections > > on > > Ctrl+C, and all running queries are cancelled before pgbench exits. > > > > In thread #0, setup_cancel_handler is called before the loop, the > > CancelRequested flag is set when Ctrl+C is issued. In the loop, cancel > > requests are sent when the flag is set only in thread #0. SIGINT is > > blocked in other threads, but the threads will exit after their query > > are cancelled. If thread safety is disabled or OS is Windows, the signal > > is not blocked because pthread_sigmask cannot be used. > > (I didn't test the patch on WIndows yet, though.) > > > > I choose the design that the signal handler and the query cancel are > > performed only in thread #0 because I wanted to make the behavior as > > predicable as possible. However, another design that all thread can > > received SIGINT and that the first thread that catches the signal is > > responsible to sent cancel requests for all connections may also work. > > > > Also, the array of CState that contains all clients state is changed to > > a global variable so that all connections can be accessed within a thread. > > > > > > +1 > I also like the thread #0 handling design. I have NOT reviewed/tested > this yet. Thank you for your comment. As a side note, actually I thought another design to create a special thread for handling query cancelling in which SIGINT would be catched by sigwait, I quit the idea because it would add complexity due to code for Windows and --disabled-thread-safe. I would appreciate it if you could kindly review and test the patch! Regards, Yugo Nagata -- Yugo NAGATA
pgbnech: allow to cancel queries during benchmark
Hello,
This attached patch enables pgbench to cancel queries during benchmark.
Formerly, Ctrl+C during benchmark killed pgbench immediately, but backend
processes executing long queries remained for a while. You can simply
reproduce this problem by cancelling the pgbench running a custom script
executing "SELECT pg_sleep(10)".
The patch fixes this so that cancel requests are sent for all connections on
Ctrl+C, and all running queries are cancelled before pgbench exits.
In thread #0, setup_cancel_handler is called before the loop, the
CancelRequested flag is set when Ctrl+C is issued. In the loop, cancel
requests are sent when the flag is set only in thread #0. SIGINT is
blocked in other threads, but the threads will exit after their query
are cancelled. If thread safety is disabled or OS is Windows, the signal
is not blocked because pthread_sigmask cannot be used.
(I didn't test the patch on WIndows yet, though.)
I choose the design that the signal handler and the query cancel are
performed only in thread #0 because I wanted to make the behavior as
predicable as possible. However, another design that all thread can
received SIGINT and that the first thread that catches the signal is
responsible to sent cancel requests for all connections may also work.
Also, the array of CState that contains all clients state is changed to
a global variable so that all connections can be accessed within a thread.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 1d1670d4c2..4e80afdf9b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -606,6 +606,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -648,6 +649,8 @@ typedef struct
* here */
} CState;
+CState *all_state; /* status of all clients */
+
/*
* Thread state
*/
@@ -3639,6 +3642,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4670,6 +4674,18 @@ disconnect_all(CState *state, int length)
finishCon([i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (all_state[i].cancel != NULL)
+ (void) PQcancel(all_state[i].cancel, errbuf, sizeof(errbuf));
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -6607,7 +6623,6 @@ main(int argc, char **argv)
bool initialization_option_set = false;
bool internal_script_used = false;
- CState *state; /* status of clients */
TState *threads; /* array of thread */
pg_time_usec_t
@@ -6656,7 +6671,7 @@ main(int argc, char **argv)
}
}
- state = (CState *) pg_malloc0(sizeof(CState));
+ all_state = (CState *) pg_malloc0(sizeof(CState));
/* set random seed early, because it may be used while parsing scripts. */
if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))
@@ -6715,7 +6730,7 @@ main(int argc, char **argv)
pg_fatal("invalid variable definition: \"%s\"", optarg);
*p++ = '\0';
- if (!putVariable([0].variables, "option", optarg, p))
+ if (!putVariable(_state[0].variables, "option", optarg, p))
exit(1);
}
break;
@@ -7087,28 +7102,28 @@ main(int argc, char **argv)
if (nclients > 1)
{
- state = (CState *) pg_realloc(state, sizeof(CState) * nclients);
- memset(state + 1, 0, sizeof(CState) * (nclients - 1));
+ all_state = (CState *) pg_realloc(all_state, sizeof(CState) * nclients);
+ memset(all_state + 1, 0, sizeof(CState) * (nclients - 1));
/* copy any -D switch values to all clients */
for (i = 1; i < nclients; i++)
{
int j;
- state[i].id = i;
- for (j = 0; j < state[0].variables.nvars; j++)
+ all_state[i].id = i;
+ for (j = 0; j < all_state[0].variables.nvars; j++)
{
-Variable *var = [0].variables.vars[j];
+Variable *var = _state[0].variables.vars[j];
if (var->value.type != PGBT_NO_VALUE)
{
- if (!putVariableValue([i].variables, "startup",
+ if (!putVariableValue(_state[i].variables, "startup",
var->name, >value))
exit(1);
}
else
{
- if (!putVariable([i].variables, "startup",
+ if (!putVariable(_state[i].variables, "startup",
var->name, var->svalue))
exit(1);
}
@@ -7119,8 +7134,8 @@ main(int argc, char **argv)
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
- state[i].cstack = conditional_stack_create();
- init
Re: Make pgbench exit on SIGINT more reliably
On Mon, 19 Jun 2023 16:49:05 -0700 "Tristan Partin" wrote: > On Mon Jun 19, 2023 at 6:39 AM PDT, Yugo NAGATA wrote: > > On Wed, 24 May 2023 08:58:46 -0500 > > "Tristan Partin" wrote: > > > > > On Tue May 23, 2023 at 7:31 PM CDT, Michael Paquier wrote: > > > > On Mon, May 22, 2023 at 10:02:02AM -0500, Tristan Partin wrote: > > > > > The way that pgbench handled SIGINT changed in > > > > > 1d468b9ad81b9139b4a0b16b416c3597925af4b0. Unfortunately this had a > > > > > couple of unintended consequences, at least from what I can tell[1]. > > > > > > > > > > - CTRL-C no longer stops the program unless the right point in pgbench > > > > > execution is hit > > > > > - pgbench no longer exits with a non-zero exit code > > > > > > > > > > An easy reproduction of these problems is to run with a large scale > > > > > factor like: pgbench -i -s 50. Then try to CTRL-C the program. > > > > > > > > This comes from the code path where the data is generated client-side, > > > > and where the current CancelRequested may not be that responsive, > > > > isn't it? > > > > > > It comes from the fact that CancelRequested is only checked within the > > > generation of the pgbench_accounts table, but with a large enough scale > > > factor or enough latency, say cross-continent communication, generating > > > the other tables can be time consuming too. But, yes you are more likely > > > to run into this issue when generating client-side data. > > > > If I understand correctly, your patch allows to exit pgbench immediately > > during a client-side data generation even while the tables other than > > pgbench_accounts are processed. It can be useful when the scale factor > > is large. However, the same purpose seems to be achieved by you other patch > > [1]. > > Is the latter your latest proposal that replaces the patch in this > > thread?ad? > > > > [1] > > https://www.postgresql.org/message-id/flat/CSTU5P82ONZ1.19XFUGHMXHBRY%40c3po > > The other patch does not replace this one. They are entirely separate. After applying the other patch, CancelRequested would be checked enough frequently even during initialization of pgbench_branches and pgbench_tellers, and it will allow the initialization step to be cancelled immediately even if the scale factor is high. So, is it unnecessary to modify setup_cancel_handler anyway? I think it would be still nice to introduce a new exit code for query cancel separately. > > > Also, your proposal includes to change the exit code when pgbench is > > cancelled by > > SIGINT. I think it is nice because this will make it easy to understand and > > clarify > > the result of the initialization. > > > > Currently, pgbench returns 0 when the initialization is cancelled while > > populating > > pgbench_branches or pgbench_tellers, but the resultant pgbench_accounts has > > only > > one row, which is an inconsistent result. Returning the specific value for > > SIGINT > > cancel can let user know it explicitly. In addition, it seems better if an > > error > > message to inform would be output. > > > > For the above purpose, the patch moved exit codes of psql to fe_utils to be > > shared. > > However, I am not sure this is good way. Each frontend-tool may have it > > own exit code, > > for example. "2" means "bad connection" in psql [2], but "errors during the > > run" in > > pgbench [3]. So, I think it is better to define them separately. > > > > [2] https://www.postgresql.org/docs/current/app-psql.html#id-1.9.4.20.7 > > [3] https://www.postgresql.org/docs/current/pgbench.html#id=id-1.9.4.11.7 > > I can change it. I just figured that sharing exit code definitions > would've been preferrable. I will post a v3 some time soon which removes > that patch. Great! > > Thanks for your review :). > > -- > Tristan Partin > Neon (https://neon.tech) -- Yugo NAGATA
Re: Make pgbench exit on SIGINT more reliably
On Wed, 24 May 2023 08:58:46 -0500 "Tristan Partin" wrote: > On Tue May 23, 2023 at 7:31 PM CDT, Michael Paquier wrote: > > On Mon, May 22, 2023 at 10:02:02AM -0500, Tristan Partin wrote: > > > The way that pgbench handled SIGINT changed in > > > 1d468b9ad81b9139b4a0b16b416c3597925af4b0. Unfortunately this had a > > > couple of unintended consequences, at least from what I can tell[1]. > > > > > > - CTRL-C no longer stops the program unless the right point in pgbench > > > execution is hit > > > - pgbench no longer exits with a non-zero exit code > > > > > > An easy reproduction of these problems is to run with a large scale > > > factor like: pgbench -i -s 50. Then try to CTRL-C the program. > > > > This comes from the code path where the data is generated client-side, > > and where the current CancelRequested may not be that responsive, > > isn't it? > > It comes from the fact that CancelRequested is only checked within the > generation of the pgbench_accounts table, but with a large enough scale > factor or enough latency, say cross-continent communication, generating > the other tables can be time consuming too. But, yes you are more likely > to run into this issue when generating client-side data. If I understand correctly, your patch allows to exit pgbench immediately during a client-side data generation even while the tables other than pgbench_accounts are processed. It can be useful when the scale factor is large. However, the same purpose seems to be achieved by you other patch [1]. Is the latter your latest proposal that replaces the patch in this thread?ad? [1] https://www.postgresql.org/message-id/flat/CSTU5P82ONZ1.19XFUGHMXHBRY%40c3po Also, your proposal includes to change the exit code when pgbench is cancelled by SIGINT. I think it is nice because this will make it easy to understand and clarify the result of the initialization. Currently, pgbench returns 0 when the initialization is cancelled while populating pgbench_branches or pgbench_tellers, but the resultant pgbench_accounts has only one row, which is an inconsistent result. Returning the specific value for SIGINT cancel can let user know it explicitly. In addition, it seems better if an error message to inform would be output. For the above purpose, the patch moved exit codes of psql to fe_utils to be shared. However, I am not sure this is good way. Each frontend-tool may have it own exit code, for example. "2" means "bad connection" in psql [2], but "errors during the run" in pgbench [3]. So, I think it is better to define them separately. [2] https://www.postgresql.org/docs/current/app-psql.html#id-1.9.4.20.7 [3] https://www.postgresql.org/docs/current/pgbench.html#id=id-1.9.4.11.7 Regards, Yugo Nagata > -- > Tristan Partin > Neon (https://neon.tech) > > -- Yugo NAGATA
Re: PG 16 draft release notes ready
On Mon, 5 Jun 2023 11:42:43 -0400 Bruce Momjian wrote: > On Mon, Jun 5, 2023 at 05:33:51PM +0900, Yugo NAGATA wrote: > > Hello, > > > > On Thu, 18 May 2023 16:49:47 -0400 > > Bruce Momjian wrote: > > > > > I have completed the first draft of the PG 16 release notes. You can > > > see the output here: > > > > Thanks for the release notes. > > > > > > > > https://momjian.us/pgsql_docs/release-16.html > > > > > > I will adjust it to the feedback I receive; that URL will quickly show > > > all updates. > > > > I didn't find the following in the release note. This might be > > considered as a bug fix, but the change in this commit can potentially > > impact applications. Is it worth including it in the release note? > > > > commit 43351557d0d2b9c5e20298b5fee2849abef86aff > > Make materialized views participate in predicate locking > > I did look at this commit and decided, thought it is a behavior change, > that it is probably something that would be caught during upgrade > testing. I thought it was rare enough, and so hard to describe about > how to adjust for it, that is should not be mentioned. If we find that > users do hit this issue, or others, during beta, we can add it. Thank you for replying. I understood. Regards, Yugo Nagata > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > Only you can decide what is important to you. -- Yugo NAGATA
Re: PG 16 draft release notes ready
Hello, On Thu, 18 May 2023 16:49:47 -0400 Bruce Momjian wrote: > I have completed the first draft of the PG 16 release notes. You can > see the output here: Thanks for the release notes. > > https://momjian.us/pgsql_docs/release-16.html > > I will adjust it to the feedback I receive; that URL will quickly show > all updates. I didn't find the following in the release note. This might be considered as a bug fix, but the change in this commit can potentially impact applications. Is it worth including it in the release note? commit 43351557d0d2b9c5e20298b5fee2849abef86aff Make materialized views participate in predicate locking Regards, Yugo Nagata > I learned a few things creating it this time: > > * I can get confused over C function names and SQL function names in >commit messages. > > * The sections and ordering of the entries can greatly clarify the >items. > > * The feature count is slightly higher than recent releases: > > release-10: 189 > release-11: 170 > release-12: 180 > release-13: 178 > release-14: 220 > release-15: 184 > --> release-16: 200 > > -- > Bruce Momjian https://momjian.us > EDB https://enterprisedb.com > > Only you can decide what is important to you. > > -- Yugo NAGATA
Re: pgbench - adding pl/pgsql versions of tests
On Fri, 24 Mar 2023 22:17:33 + Cary Huang wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: tested, passed > Spec compliant: not tested > Documentation:not tested The patch would need documentations describing the new options. > > Hi > > thank you for the patch. It can be applied to current master branch and > compiled fine. I also confirmed that it can be applied and complied, although it raised warnings about whitespace errors. /tmp/pgbench-plpgsql-001.patch:68: trailing whitespace. executeStatement(con, /tmp/pgbench-plpgsql-001.patch:102: trailing whitespace. executeStatement(con, warning: 2 lines add whitespace errors. > The feature works as described, I am able to run plpgsql-tpcb-like and > plpgsql-simple-update scripts as you have added them. > > But I am not sure the purpose of --no-function to prevent the creation of > pl/pgsql functions when the new plpgsql test scripts need them. > > I initialized pgbench database with --no-function, and plpgsql-tpcb-like and > plpgsql-simple-update scripts will fail to run I am not sure either whether --no-function option is necessary. Although there is --no-vacuum, I guess this would be intended to reduce the initialization time. I don't think omitting creating functions has such effect. So, I wonder -no-function is unnecessary, as similar to that there are no option to omitting to create tables. Regards, Yugo Nagata -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Thu, 1 Jun 2023 23:59:09 +0900 Yugo NAGATA wrote: > Hello hackers, > > Here's a rebased version of the patch-set adding Incremental View > Maintenance support for PostgreSQL. That was discussed in [1]. > [1] > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp --- * Overview Incremental View Maintenance (IVM) is a way to make materialized views up-to-date by computing only incremental changes and applying them on views. IVM is more efficient than REFRESH MATERIALIZED VIEW when only small parts of the view are changed. ** Feature The attached patchset provides a feature that allows materialized views to be updated automatically and incrementally just after a underlying table is modified. You can create an incementally maintainable materialized view (IMMV) by using CREATE INCREMENTAL MATERIALIZED VIEW command. The followings are supported in view definition queries: - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) - some built-in aggregate functions (count, sum, avg, min, max) - GROUP BY clause - DISTINCT clause Views can contain multiple tuples with the same content (duplicate tuples). ** Restriction The following are not supported in a view definition: - Outer joins - Aggregates otehr than above, window functions, HAVING - Sub-queries, CTEs - Set operations (UNION, INTERSECT, EXCEPT) - DISTINCT ON, ORDER BY, LIMIT, OFFSET Also, a view definition query cannot contain other views, materialized views, foreign tables, partitioned tables, partitions, VALUES, non-immutable functions, system columns, or expressions that contains aggregates. --- * Design An IMMV is maintained using statement-level AFTER triggers. When an IMMV is created, triggers are automatically created on all base tables contained in the view definition query. When a table is modified, changes that occurred in the table are extracted as transition tables in the AFTER triggers. Then, changes that will occur in the view are calculated by a rewritten view dequery in which the modified table is replaced with the transition table. For example, if the view is defined as "SELECT * FROM R, S", and tuples inserted into R are stored in a transiton table dR, the tuples that will be inserted into the view are calculated as the result of "SELECT * FROM dR, S". ** Multiple Tables Modification Multiple tables can be modified in a statement when using triggers, foreign key constraint, or modifying CTEs. When multiple tables are modified, we need the state of tables before the modification. For example, when some tuples, dR and dS, are inserted into R and S respectively, the tuples that will be inserted into the view are calculated by the following two queries: "SELECT * FROM dR, S_pre" "SELECT * FROM R, dS" where S_pre is the table before the modification, R is the current state of table, that is, after the modification. This pre-update states of table is calculated by filtering inserted tuples and appending deleted tuples. The subquery that represents pre-update state is generated in get_prestate_rte(). Specifically, the insterted tuples are filtered by calling IVM_visible_in_prestate() in WHERE clause. This function checks the visibility of tuples by using the snapshot taken before table modification. The deleted tuples are contained in the old transition table, and this table is appended using UNION ALL. Transition tables for each modification are collected in each AFTER trigger function call. Then, the view maintenance is performed in the last call of the trigger. In the original PostgreSQL, tuplestores of transition tables are freed at the end of each nested query. However, their lifespan needs to be prolonged to the end of the out-most query in order to maintain the view in the last AFTER trigger. For this purpose, SetTransitionTablePreserved is added in trigger.c. ** Duplicate Tulpes When calculating changes that will occur in the view (= delta tables), multiplicity of tuples are calculated by using count(*). When deleting tuples from the view, tuples to be deleted are identified by joining the delta table with the view, and tuples are deleted as many as specified multiplicity by numbered using row_number() function. This is implemented in apply_old_delta(). When inserting tuples into the view, each tuple is duplicated to the specified multiplicity using generate_series() function. This is implemented in apply_new_delta(). ** DISTINCT clause When DISTINCT is used, the view has a hidden column __ivm_count__ that stores multiplicity for tuples. When tuples are deleted from or inserted into the view, the values of __ivm_count__ column is decreased or increased as many as specified multiplicity. Eventually, when the va
Using Ephemeral Named Relation like a temporary table
Hello,
Temporary tables are often used to store transient data in
batch processing and the contents can be accessed multiple
times. However, frequent use of temporary tables has a problem
that the system catalog tends to bloat. I know there has been
several proposals to attack this problem, but I would like to
propose a new one.
The idea is to use Ephemeral Named Relation (ENR) like a
temporary table. ENR information is not stored into the system
catalog, but in QueryEnvironment, so it never bloat the system
catalog.
Although we cannot perform insert, update or delete on ENR,
I wonder it could be beneficial if we need to reference to a
result of a query multiple times in a batch processing.
The attached is a concept patch. This adds a new syntax
"OPEN cursor INTO TABLE tablename" to pl/pgSQL, that stores
a result of the cursor query into a ENR with specified name.
However, this is a tentative interface to demonstrate the
concept of feature.
Here is an example;
postgres=# \sf fnc
CREATE OR REPLACE FUNCTION public.fnc()
RETURNS TABLE(sum1 integer, avg1 integer, sum2 integer, avg2 integer)
LANGUAGE plpgsql
AS $function$
DECLARE
sum1 integer;
sum2 integer;
avg1 integer;
avg2 integer;
curs CURSOR FOR SELECT aid, bid, abalance FROM pgbench_accounts
WHERE abalance BETWEEN 100 AND 200;
BEGIN
OPEN curs INTO TABLE tmp_accounts;
SELECT count(abalance) , avg(abalance) INTO sum1, avg1
FROM tmp_accounts;
SELECT count(bbalance), avg(bbalance) INTO sum2, avg2
FROM tmp_accounts a, pgbench_branches b WHERE a.bid = b.bid;
RETURN QUERY SELECT sum1,avg1,sum2,avg2;
END;
$function$
postgres=# select fnc();
fnc
(541,151,541,3937)
(1 row)
As above, we can use the same query result for multiple
aggregations, and also join it with other tables.
What do you think of using ENR for this way?
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index e3a170c38b..27e94ecc87 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -20,6 +20,7 @@
#include "access/xact.h"
#include "catalog/heap.h"
#include "catalog/pg_type.h"
+#include "commands/portalcmds.h"
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/spi_priv.h"
@@ -3378,3 +3379,30 @@ SPI_register_trigger_data(TriggerData *tdata)
return SPI_OK_TD_REGISTER;
}
+
+int
+SPI_register_portal(Portal portal, char *name)
+{
+ int rc;
+ EphemeralNamedRelation enr;
+
+ if (portal == NULL || name == NULL)
+ return SPI_ERROR_ARGUMENT;
+
+ PortalCreateHoldStore(portal);
+ PersistHoldablePortal(portal);
+
+ enr = palloc(sizeof(EphemeralNamedRelationData));
+
+ enr->md.name = name;
+ enr->md.reliddesc = InvalidOid;
+ enr->md.tupdesc = portal->tupDesc;
+ enr->md.enrtype = ENR_NAMED_TUPLESTORE;
+ enr->md.enrtuples = tuplestore_tuple_count(portal->holdStore);
+ enr->reldata = portal->holdStore;
+ rc = SPI_register_relation(enr);
+ if (rc != SPI_OK_REL_REGISTER)
+ return rc;
+
+ return SPI_OK_TD_REGISTER;
+}
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index d1de139a3b..40753dc78a 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -199,6 +199,7 @@ extern void SPI_cursor_close(Portal portal);
extern int SPI_register_relation(EphemeralNamedRelation enr);
extern int SPI_unregister_relation(const char *name);
extern int SPI_register_trigger_data(TriggerData *tdata);
+extern int SPI_register_portal(Portal portal, char *name);
extern void SPI_start_transaction(void);
extern void SPI_commit(void);
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b0a2cac227..5d561b39bd 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4776,6 +4776,9 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
elog(ERROR, "could not open cursor: %s",
SPI_result_code_string(SPI_result));
+ if (stmt->tablename)
+ SPI_register_portal(portal, stmt->tablename);
+
/*
* If cursor variable was NULL, store the generated portal name in it,
* after verifying it's okay to assign to.
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index edeb72c380..576ea86943 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -185,7 +185,7 @@ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%type foreach_slice
%type for_control
-%type any_identifier opt_block_label opt_loop_label opt_label
+%type any_identifier opt_block_label opt_into_table opt_loop_label opt_label
%type option_value
%type proc_sect stmt_elsifs stmt_else
@@ -2064,7 +2064,17 @@ stmt_dynexecute : K_EXECUTE
;
-stmt_open : K_OPEN cursor_variable
+opt_into_table :
+ {
+ $$ = NULL;
+ }
+|
pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
Hello,
Attached patch introduces a function pg_column_toast_chunk_id
that returns a chunk ID of a TOASTed value.
Recently, one of our clients needed a way to show which columns
are actually TOASTed because they would like to know how much
updates on the original table affects to its toast table
specifically with regard to auto VACUUM. We could not find a
function for this purpose in the current PostgreSQL, so I would
like propose pg_column_toast_chunk_id.
This function returns a chunk ID of a TOASTed value, or NULL
if the value is not TOASTed. Here is an example;
postgres=# \d val
Table "public.val"
Column | Type | Collation | Nullable | Default
+--+---+--+-
t | text | | |
postgres=# select length(t), pg_column_size(t), pg_column_compression(t),
pg_column_toast_chunk_id(t), tableoid from val;
length | pg_column_size | pg_column_compression | pg_column_toast_chunk_id |
tableoid
++---+--+--
3 | 4 | | |
16388
3000 | 46 | pglz | |
16388
32000 |413 | pglz | |
16388
305 |309 | | |
16388
64000 | 64000 | | 16393 |
16388
(5 rows)
postgres=# select chunk_id, chunk_seq from pg_toast.pg_toast_16388;
chunk_id | chunk_seq
--+---
16393 | 0
16393 | 1
16393 | 2
(snip)
16393 |30
16393 |31
16393 |32
(33 rows)
This function is also useful to identify a problematic row when
an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
The patch is a just a concept patch and not including documentation
and tests.
What do you think about this feature?
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 5778e3f0ef..4ddcf885cd 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5070,6 +5070,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk id of the TOASTed value.
+ * Return NULL for unTOASTed value.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7c358cff16..4c214f4c63 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7414,6 +7414,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of TOASTed value',
+ proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype => 'oid',
+ proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
{ oid => '2322',
descr => 'total disk space usage for the specified tablespace',
proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
Re: psql \watch 2nd argument: iteration count
Hello,
On Thu, 16 Mar 2023 21:15:30 -0700
Andrey Borodin wrote:
> On Wed, Mar 15, 2023 at 5:54 PM Michael Paquier wrote:
> >
> > On Wed, Mar 15, 2023 at 04:58:49PM +0900, Michael Paquier wrote:
> > > Yep. You are right.
> >
> > Fixed that and applied 0001.
> Great, thanks!
>
> >
> > +valptr++;
> > +if (strncmp("i", opt, strlen("i")) == 0 ||
> > +strncmp("interval", opt, strlen("interval")) == 0)
> > +{
> >
> > Did you look at process_command_g_options() and if some consolidation
> > was possible? It would be nice to have APIs shaped so as more
> > sub-commands could rely on the same facility in the future.
> I've tried, but they behave so differently. I could reuse only the
> "char *valptr = strchr(opt, '=');" thing from there :)
> And process_command_g_options() changes data in-place...
> Actually, I'm not sure having "i" == "interval" and "c"=="count" is a
> good idea here too. I mean I like it, but is it coherent?
> Also I do not like repeating 4 times this 5 lines
> + pg_log_error("\\watch: incorrect interval value '%s'", valptr);
> + free(opt);
> + resetPQExpBuffer(query_buf);
> + psql_scan_reset(scan_state);
> + return PSQL_CMD_ERROR;
> But I hesitate defining a new function for this...
>
> >
> > -\watch [ > class="parameter">seconds ]
> > +\watch [ > class="parameter">seconds [ > class="parameter">iterations ] ]
> >
> > This set of changes is not reflected in the documentation.
> Done.
>
> > With an interval in place, we could now automate some tests with
> > \watch where it does not fail. What do you think about adding a test
> > with a simple query, an interval of 0s and one iteration?
> Done. Also found a bug that we actually were doing N+1 iterations.
Here is my review on the v9 patch.
+ /* we do not prevent numerous names iterations like i=1 i=1
i=1 */
+ have_sleep = true;
Why this is allowed here? I am not sure there is any reason to allow to specify
multiple "interval" options. (I would apologize it if I missed past discussion.)
+ if (sleep < 0 || *opt_end || errno == ERANGE || have_sleep)
+ {
+ pg_log_error("\\watch: incorrect interval value '%s'",
Here, specifying an explicit "interval" option before an interval second without
option is prohibited.
postgres=# select 1 \watch interval=3 4
\watch: incorrect interval value '4'
I think it is ok, but this error message seems not user-friendly because,
in the above example, interval values itself is correct, but it seems just
a syntax error. I wonder it is better to use "watch interval must be specified
only once" or such here, as the past patch.
+
+If number of iterations is specified - query will be executed only
+given number of times.
+
Is it common to use "-" here? I think using comma like
"If number of iterations is specified, "
is natural.
Regards,
Yugo Nagata
--
Yugo NAGATA
