Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
On Wed, Jan 06, 2021 at 11:28:36AM +0500, Andrey Borodin wrote:
> First patch fixes a bug with possible SLRU truncation around wrapping point
> too early.
> Basic idea of the patch is "If we want to delete a range we must be eligible
> to delete it's beginning and ending".
> So to test if page is deletable it tests that first and last xids(or other
> SLRU's unit) are of no interest. To test if a segment is deletable it tests
> if first and last pages can be deleted.
Yes.
> I'm a little suspicious of implementation of *PagePrecedes(int page1, int
> page2) functions. Consider following example from the patch:
>
> static bool
> CommitTsPagePrecedes(int page1, int page2)
> {
> TransactionId xid1;
> TransactionId xid2;
>
> xid1 = ((TransactionId) page1) * COMMIT_TS_XACTS_PER_PAGE;
> xid1 += FirstNormalTransactionId + 1;
> xid2 = ((TransactionId) page2) * COMMIT_TS_XACTS_PER_PAGE;
> xid2 += FirstNormalTransactionId + 1;
>
> return (TransactionIdPrecedes(xid1, xid2) &&
> TransactionIdPrecedes(xid1, xid2 + COMMIT_TS_XACTS_PER_PAGE - 1));
> }
>
> We are adding FirstNormalTransactionId to xids to avoid them being special
> xids.
> We add COMMIT_TS_XACTS_PER_PAGE - 1 to xid2 to shift it to the end of the
> page. But due to += FirstNormalTransactionId we shift slightly behind page
> boundaries and risk that xid2 + COMMIT_TS_XACTS_PER_PAGE - 1 can become
> FrozenTransactionId (FirstNormalTransactionId - 1). Thus we add +1 to all
> values in scope. While the logic is correct, coding is difficult. Maybe we
> could just use
Right. The overall objective is to compare the first XID of page1 to the
first and last XIDs of page2. The FirstNormalTransactionId+1 addend operates
at a lower level. It just makes TransactionIdPrecedes() behave like
NormalTransactionIdPrecedes() without the latter's assertion.
> page1_first_normal_xid = ((TransactionId) page1) * CLOG_XACTS_PER_PAGE
> + FirstNormalTransactionId;
> page2_first_normal_xid = ((TransactionId) page2) * CLOG_XACTS_PER_PAGE
> + FirstNormalTransactionId;
> page2_last_xid = ((TransactionId) page2) * CLOG_XACTS_PER_PAGE +
> CLOG_XACTS_PER_PAGE - 1;
> But I'm not insisting on this.
I see your point, but I doubt using different addends on different operands
makes this easier to understand. If anything, I'd lean toward adding more
explicit abstraction between "the XID we intend to test" and "the XID we're
using to fool some general-purpose API".
> Following comment is not correct for 1Kb and 4Kb pages
> + * At every supported BLCKSZ, (1 << 31) % COMMIT_TS_XACTS_PER_PAGE == 128.
Fixed, thanks.
> All above notes are not blocking the fix, I just wanted to let committer know
> about this. I think that it's very important to have this bug fixed.
>
> Second patch converts binary *PagePreceeds() functions to *PageDiff()s and
> adds logic to avoid deleting pages in suspicious cases. This logic depends on
> the scale of returned by diff value: it expects that overflow happens between
> INT_MIN\INT_MAX. This it prevents page deletion if page_diff <= INT_MIN / 2
> (too far from current cleaning point; and in normal cases, of cause).
>
> It must be comparison here, not equality test.
> - ctl->PagePrecedes(segpage, trunc->earliestExistingPage))
> + ctl->PageDiff(segpage, trunc->earliestExistingPage))
That's bad. Fixed, thanks.
> This
> int diff_max = ((QUEUE_MAX_PAGE + 1) / 2) - 1,
> seems to be functional equivalent of
> int diff_max = ((QUEUE_MAX_PAGE - 1) / 2),
Do you think one conveys the concept better than the other?
> AFAICS the overall purpose of the 2nd patch is to help corrupted by other
> bugs clusters avoid deleting SLRU segments.
Yes.
> I'm a little bit afraid that this kind of patch can hide bugs (while
> potentially saving some users data). Besides this patch seems like a useful
> precaution. Maybe we could emit scary warnings if SLRU segments do not stack
> into continuous range?
Scary warnings are good for an observation that implies a bug, but the
slru-truncate-t-insurance patch causes such an outcome in non-bug cases where
it doesn't happen today. In other words, discontinuous ranges of SLRU
segments would be even more common after that patch. For example, it would
happen anytime oldestXID advances by more than ~1B at a time.
Thanks,
nm
pg_dump vs. GRANT OPTION among initial privileges
pg_dump can generate invalid SQL syntax if an aclitem in pg_init_privs
contains a GRANT OPTION (an asterisk in the aclitemout() representation).
Also, pg_dump uses REVOKE GRANT OPTION FOR where it needs plain REVOKE. For
more details, see the log message in the attached patch. I recommend
reviewing "diff -w" output first. I'm not certain the test suite will always
find those REVOKE statements in the same order; if that order varies, one
regex will need more help.
Author: Noah Misch
Commit: Noah Misch
Fix pg_dump for GRANT OPTION among initial privileges.
The context is an object that no longer bears some aclitem that it bore
initially. (A user issued REVOKE or GRANT statements upon the object.)
pg_dump is forming SQL to reproduce the object ACL. Since initdb
creates no ACL bearing GRANT OPTION, reaching this bug requires an
extension where the creation script establishes such an ACL. No PGXN
extension does that. If an installation did reach the bug, pg_dump
would have omitted a semicolon, causing a REVOKE and the next SQL
statement to fail. Separately, since the affected code exists to
eliminate an entire aclitem, it wants plain REVOKE, not REVOKE GRANT
OPTION FOR. Back-patch to 9.6, where commit
23f34fa4ba358671adab16773e79c17c92cbc870 first appeared.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 536c9ff..2129301 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -168,48 +168,28 @@ buildACLCommands(const char *name, const char *subname,
const char *nspname,
for (i = 0; i < nraclitems; i++)
{
if (!parseAclItem(raclitems[i], type, name, subname,
remoteVersion,
- grantee, grantor,
privs, privswgo))
+ grantee, grantor,
privs, NULL))
{
ok = false;
break;
}
- if (privs->len > 0 || privswgo->len > 0)
+ if (privs->len > 0)
{
- if (privs->len > 0)
- {
- appendPQExpBuffer(firstsql, "%sREVOKE
%s ON %s ",
-
prefix, privs->data, type);
- if (nspname && *nspname)
- appendPQExpBuffer(firstsql,
"%s.", fmtId(nspname));
- appendPQExpBuffer(firstsql, "%s FROM ",
name);
- if (grantee->len == 0)
- appendPQExpBufferStr(firstsql,
"PUBLIC;\n");
- else if (strncmp(grantee->data, "group
",
-
strlen("group ")) == 0)
- appendPQExpBuffer(firstsql,
"GROUP %s;\n",
-
fmtId(grantee->data + strlen("group ")));
- else
- appendPQExpBuffer(firstsql,
"%s;\n",
-
fmtId(grantee->data));
- }
- if (privswgo->len > 0)
- {
- appendPQExpBuffer(firstsql,
-
"%sREVOKE GRANT OPTION FOR %s ON %s ",
-
prefix, privswgo->data, type);
- if (nspname && *nspname)
- appendPQExpBuffer(firstsql,
"%s.", fmtId(nspname));
- appendPQExpBuffer(firstsql, "%s FROM ",
name);
- if (grantee->len == 0)
- appendPQExpBufferStr(firstsql,
"PUBLIC");
- else if (strncmp(grantee->data, "group
",
-
strlen("group ")) == 0)
- appendPQExpBuffer(firstsql,
"GROUP %s",
-
fmtId(grantee->data + strlen("group ")));
- else
- appendPQExpBufferStr(firstsql,
fmtId(grantee->data));
-
Re: WIP: System Versioned Temporal Table
On Fri, Jan 8, 2021 at 9:19 PM Ryan Lambert wrote: >> Updated v11 with additional docs and some rewording of messages/tests >> to use "system versioning" correctly. >> >> No changes on the points previously raised. >> > Thank you! The v11 applies and installs. I tried a simple test, > unfortunately it appears the versioning is not working. The initial value is > not preserved through an update and a new row does not appear to be created. Agreed. I already noted this in my earlier review comments. I will send in a new version with additional tests so we can see more clearly that the tests fail on the present patch. I will post more on this next week. -- Simon Riggshttp://www.EnterpriseDB.com/
Re: Implement for window functions
Hi, the building warning below is fixed now, no other changes. Also, I can
confirm that the corner case with offset=0 in lead and lag works correctly.
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -O2 -I../../../src/include
-I/home/krasiyan/pgsql/postgresql/src/include -D_GNU_SOURCE
-I/usr/include/libxml2 -c -o nodeWindowAgg.o
/home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c
/home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
function ‘WinGetFuncArgInPartition’:
/home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3274:10:
warning: ‘step’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
3274 | relpos += step;
| ~~~^~~
/home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
function ‘WinGetFuncArgInFrame’:
/home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3531:10:
warning: ‘step’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
3531 | relpos += step;
| ~~~^~~
На пт, 8.01.2021 г. в 2:02 ч. Vik Fearing написа:
> On 1/1/21 10:21 PM, Zhihong Yu wrote:
> > Krasiyan:
> > Happy New Year.
> >
> > For WinGetFuncArgInPartition():
> >
> > + if (target > 0)
> > + step = 1;
> > + else if (target < 0)
> > + step = -1;
> > + else
> > + step = 0;
> >
> > When would the last else statement execute ? Since the above code is
> > for WINDOW_SEEK_CURRENT, I wonder why step should be 0.
>
> Hi.
>
> "lag(expr, 0) over w" is useless but valid.
> --
> Vik Fearing
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 02a37658ad..7a6f194138 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -20402,6 +20402,14 @@ SELECT count(*) FROM sometable;
about frame specifications.
+
+ The functions lead, lag,
+ first_value, last_value, and
+ nth_value accept a null treatment option which is
+ RESPECT NULLS or IGNORE NULLS.
+ If this option is not specified, the default is RESPECT NULLS.
+
+
When an aggregate function is used as a window function, it aggregates
over the rows within the current row's window frame.
@@ -20415,14 +20423,9 @@ SELECT count(*) FROM sometable;
-The SQL standard defines a RESPECT NULLS or
-IGNORE NULLS option for lead, lag,
-first_value, last_value, and
-nth_value. This is not implemented in
-PostgreSQL: the behavior is always the
-same as the standard's default, namely RESPECT NULLS.
-Likewise, the standard's FROM FIRST or FROM LAST
-option for nth_value is not implemented: only the
+The SQL standard defines a FROM FIRST or FROM LAST
+option for nth_value. This is not implemented in
+PostgreSQL: only the
default FROM FIRST behavior is supported. (You can achieve
the result of FROM LAST by reversing the ORDER BY
ordering.)
diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..31e08c26b4 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -28,6 +28,7 @@ CREATE [ OR REPLACE ] FUNCTION
{ LANGUAGE lang_name
| TRANSFORM { FOR TYPE type_name } [, ... ]
| WINDOW
+| TREAT NULLS
| IMMUTABLE | STABLE | VOLATILE | [ NOT ] LEAKPROOF
| CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
| [ EXTERNAL ] SECURITY INVOKER | [ EXTERNAL ] SECURITY DEFINER
@@ -293,6 +294,17 @@ CREATE [ OR REPLACE ] FUNCTION
+
+ TREAT NULLS
+
+
+ TREAT NULLS indicates that the function is able
+ to handle the RESPECT NULLS and IGNORE
+ NULLS options. Only window functions may specify this.
+
+
+
+
IMMUTABLE
STABLE
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index d66560b587..103595d21b 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -1766,6 +1766,8 @@ FROM generate_series(1,10) AS s(i);
The syntax of a window function call is one of the following:
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER window_name
+function_name (expression , expression ... ) [ RESPECT NULLS | IGNORE NULLS ] OVER ( window_definition )
function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition )
function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER window_name
@@ -1779,6 +1781,18 @@ FROM generate_series(1,10) AS s(i);
[ ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [,
Re: Added schema level support for publication.
On Fri, Jan 8, 2021 at 4:32 PM Amit Kapila wrote: > > On Thu, Jan 7, 2021 at 10:03 PM vignesh C wrote: > > > > This feature adds schema option while creating publication. Users will > > be able to specify one or more schemas while creating publication, > > when the user specifies schema option, then the data changes for the > > tables present in the schema specified by the user will be replicated > > to the subscriber. Few examples have been listed below: > > > > Create a publication that publishes all changes for all the tables > > present in production schema: > > CREATE PUBLICATION production_publication FOR ALL TABLES SCHEMA production; > > > > Create a publication that publishes all changes for all the tables > > present in marketing and sales schemas: > > CREATE PUBLICATION sales_publication FOR ALL TABLES SCHEMA marketing, sales; > > > > Add some schemas to the publication: > > ALTER PUBLICATION sales_publication ADD SCHEMA marketing_june, sales_june; > > > > Drop some schema from the publication: > > ALTER PUBLICATION production_quarterly_publication DROP SCHEMA > > production_july; > > > > Attached is a POC patch for the same. I felt this feature would be quite > > useful. > > > > What do we do if the user Drops the schema? Do we automatically remove > it from the publication? > I have not yet handled this scenario yet, I will handle this and adding of tests in the next patch. > I see some use of such a feature but you haven't described the use > case or how did you arrive at the conclusion that it would be quite > useful? > Currently there are a couple of options "FOR All TABLES" and "FOR TABLE" when a user creates a publication, 1) either to subscribe to the changes of all the tables or 2) subscribe to a few tables. There is no option for users to subscribe to relations present in the schemas. User has to manually identify the list of tables present in the schema and specify the list of tables in that schema using the "FOR TABLE" option. Similarly if a user wants to subscribe to n number of schemas, the user has to do this for the required schemas, this is a tedious process. This feature helps the user to take care of this internally using schema option. Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] logical decoding of two-phase transactions
On Sat, Jan 9, 2021 at 12:57 PM Amit Kapila wrote: > > On Tue, Jan 5, 2021 at 4:26 PM Amit Kapila wrote: > > > > On Tue, Jan 5, 2021 at 2:11 PM Ajin Cherian wrote: > > > > > > > > > I've addressed the above comments and the patch is attached. I've > > > called it v36-0007. > > > > > > > Thanks, I have pushed this after minor wordsmithing. > > > > The test case is failing on one of the build farm machines. See email > from Tom Lane [1]. The symptom clearly shows that we are decoding > empty xacts which can happen due to background activity by autovacuum. > I think we need a fix similar to what we have done in > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=82a0ba7707e010a29f5fe1a0020d963c82b8f1cb. > > I'll try to reproduce and provide a fix for this later today or tomorrow. > I have pushed the fix. -- With Regards, Amit Kapila.
Re: Improper use about DatumGetInt32
On 2021-01-09 02:46, Michael Paquier wrote:
+/* LCOV_EXCL_START */
Does it really make sense to add those markers here? It seems to me
that we would ignore any new coverage if regression tests based on
older versions are added (we may really want to have such tests for
more in-core extensions to be able to verify the portability of an
extension, but that's not the job of this patch of course).
If we had a way to do such testing then we wouldn't need these markers.
But AFAICT, we don't.
- elog(ERROR, "block 0 is a meta page");
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("block 0 is a meta page")));
[...]
+errmsg("block number %llu is out of range for relation \"%s\"",
This does not follow the usual style for error reports that should not
be written as full sentences? Maybe something like "invalid block
number %u referring to meta page" and "block number out of range for
relation %s: %llu"?
There are many error messages that say "[something] is out of range". I
don't think banning that would serve any purpose.
Re: Add table access method as an option to pgbench
>+ " create tables with using >specified table access method,\n" In my opinion, this sentence should be "create tables with specified table access method" or "create tables using specified table access method". "create tables with specified table access method" may be more consistent with other options.
Re: Added schema level support for publication.
On Sat, Jan 9, 2021 at 5:21 PM vignesh C wrote: > > What do we do if the user Drops the schema? Do we automatically remove > > it from the publication? > > > I have not yet handled this scenario yet, I will handle this and > adding of tests in the next patch. > > > I see some use of such a feature but you haven't described the use > > case or how did you arrive at the conclusion that it would be quite > > useful? > > > Currently there are a couple of options "FOR All TABLES" and "FOR > TABLE" when a user creates a publication, 1) either to subscribe to > the changes of all the tables or 2) subscribe to a few tables. There > is no option for users to subscribe to relations present in the > schemas. User has to manually identify the list of tables present in > the schema and specify the list of tables in that schema using the > "FOR TABLE" option. Similarly if a user wants to subscribe to n number > of schemas, the user has to do this for the required schemas, this is > a tedious process. This feature helps the user to take care of this > internally using schema option. I think this feature can be useful, in case a user has a lot of tables to publish inside a schema. Having said that, I wonder if this feature mandates users to create the same schema with same permissions/authorizations manually on the subscriber, because logical replication doesn't propagate any ddl's so are the schema or schema changes? Or is it that the list of tables from the publisher can go into a different schema on the subscriber? Since the schema can have other objects such as data types, functions, operators, I'm sure with your feature, non-table objects will be skipped. As Amit pointed out earlier, the behaviour when schema dropped, I think we should also consider when schema is altered, say altered to a different name, maybe we should change that in the publication too. In general, what happens if we have some temporary tables or foreign tables inside the schema, will they be allowed to send the data to subscribers? And, with this feature, since there can be many huge tables inside a schema, the initial table sync phase of the replication can take a while. Say a user has created a publication for a schema with hundreds of tables in it, at some point later, can he stop replicating a single or some tables from that schema? IMO, it's better to have the syntax - CREATE PUBLICATION production_publication FOR ALL TABLES IN SCHEMA production - just added IN between for all tables and schema. Say a user has a schema with 121 tables in it, and wants to replicate only 120 or 199 or even lesser tables out of it, so can we have some skip option to the new syntax, something like below? CREATE PUBLICATION production_publication FOR ALL TABLES SCHEMA production WITH skip = marketing, accounts, sales; --> meaning is, replicate all the tables in the schema production except marketing, accounts, sales tables. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Added schema level support for publication.
On Sat, Jan 9, 2021 at 8:08 PM Bharath Rupireddy wrote: > I think this feature can be useful, in case a user has a lot of tables > to publish inside a schema. Having said that, I wonder if this feature > mandates users to create the same schema with same > permissions/authorizations manually on the subscriber, because logical > replication doesn't propagate any ddl's so are the schema or schema > changes? Or is it that the list of tables from the publisher can go > into a different schema on the subscriber? > > Since the schema can have other objects such as data types, functions, > operators, I'm sure with your feature, non-table objects will be > skipped. > > As Amit pointed out earlier, the behaviour when schema dropped, I > think we should also consider when schema is altered, say altered to a > different name, maybe we should change that in the publication too. > > In general, what happens if we have some temporary tables or foreign > tables inside the schema, will they be allowed to send the data to > subscribers? > > And, with this feature, since there can be many huge tables inside a > schema, the initial table sync phase of the replication can take a > while. > > Say a user has created a publication for a schema with hundreds of > tables in it, at some point later, can he stop replicating a single or > some tables from that schema? > > IMO, it's better to have the syntax - CREATE PUBLICATION > production_publication FOR ALL TABLES IN SCHEMA production - just > added IN between for all tables and schema. > > Say a user has a schema with 121 tables in it, and wants to replicate > only 120 or 199 or even lesser tables out of it, so can we have some > skip option to the new syntax, something like below? > CREATE PUBLICATION production_publication FOR ALL TABLES SCHEMA > production WITH skip = marketing, accounts, sales; --> meaning is, > replicate all the tables in the schema production except marketing, > accounts, sales tables. One more point - if the publication is created for a schema with no or some initial tables, will all the future tables that may get added to the schema will be replicated too? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Implement for window functions
Hi, patch applies and compiles, all included and external tests and building of the docs pass. After the last run of the cfbot, there are no any building warnings. I am using last version in our testing environment with real data and I didn't find any bugs, so I'm marking this patch as ready for the committer in the commitfest app. На сб, 9.01.2021 г. в 13:30 ч. Krasiyan Andreev написа: > Hi, the building warning below is fixed now, no other changes. Also, I can > confirm that the corner case with offset=0 in lead and lag works correctly. > > gcc -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Werror=vla -Wendif-labels > -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type > -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard > -Wno-format-truncation -Wno-stringop-truncation -O2 -I../../../src/include > -I/home/krasiyan/pgsql/postgresql/src/include -D_GNU_SOURCE > -I/usr/include/libxml2 -c -o nodeWindowAgg.o > /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c > /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In > function ‘WinGetFuncArgInPartition’: > /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3274:10: > warning: ‘step’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > 3274 | relpos += step; > | ~~~^~~ > /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In > function ‘WinGetFuncArgInFrame’: > /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3531:10: > warning: ‘step’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > 3531 | relpos += step; > | ~~~^~~ > > > > На пт, 8.01.2021 г. в 2:02 ч. Vik Fearing > написа: > >> On 1/1/21 10:21 PM, Zhihong Yu wrote: >> > Krasiyan: >> > Happy New Year. >> > >> > For WinGetFuncArgInPartition(): >> > >> > + if (target > 0) >> > + step = 1; >> > + else if (target < 0) >> > + step = -1; >> > + else >> > + step = 0; >> > >> > When would the last else statement execute ? Since the above code is >> > for WINDOW_SEEK_CURRENT, I wonder why step should be 0. >> >> Hi. >> >> "lag(expr, 0) over w" is useless but valid. >> -- >> Vik Fearing >> >
Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
> 9 янв. 2021 г., в 15:17, Noah Misch написал(а): > >> This >> int diff_max = ((QUEUE_MAX_PAGE + 1) / 2) - 1, >> seems to be functional equivalent of >> int diff_max = ((QUEUE_MAX_PAGE - 1) / 2), > > Do you think one conveys the concept better than the other? I see now that next comments mention "(QUEUE_MAX_PAGE+1)/2", so I think there is no need to change something in a patch here. >> I'm a little bit afraid that this kind of patch can hide bugs (while >> potentially saving some users data). Besides this patch seems like a useful >> precaution. Maybe we could emit scary warnings if SLRU segments do not stack >> into continuous range? > > Scary warnings are good for an observation that implies a bug, but the > slru-truncate-t-insurance patch causes such an outcome in non-bug cases where > it doesn't happen today. In other words, discontinuous ranges of SLRU > segments would be even more common after that patch. For example, it would > happen anytime oldestXID advances by more than ~1B at a time. Uhm, I thought that if there is going to be more than ~1B xids - we are going to keep all segements forever and range still will be continuous. Or am I missing something? Thanks! Best regards, Andrey Borodin.
Re: Implement for window functions
Hi,
For WinGetFuncArgInFrame():
+ if (winobj->null_treatment == NULL_TREATMENT_IGNORE)
{
...
+ if (target > 0)
+ step = 1;
+ else if (target < 0)
+ step = -1;
+ else if (seektype == WINDOW_SEEK_HEAD)
+ step = 1;
+ else if (seektype == WINDOW_SEEK_TAIL)
+ step = -1;
+ else
+ step = 0;
...
+ relpos = 0;
+ }
Why is relpos always set to 0 ?
In similar code in WinGetFuncArgInPartition(), I saw the following:
+ if (target > 0)
+ step = 1;
+ else if (target < 0)
+ step = -1;
+ else
+ step = 0;
+ relpos = step;
Maybe add a comment above the relpos assignment.
Thanks
On Sat, Jan 9, 2021 at 3:31 AM Krasiyan Andreev wrote:
> Hi, the building warning below is fixed now, no other changes. Also, I can
> confirm that the corner case with offset=0 in lead and lag works correctly.
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> -Wno-format-truncation -Wno-stringop-truncation -O2 -I../../../src/include
> -I/home/krasiyan/pgsql/postgresql/src/include -D_GNU_SOURCE
> -I/usr/include/libxml2 -c -o nodeWindowAgg.o
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
> function ‘WinGetFuncArgInPartition’:
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3274:10:
> warning: ‘step’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 3274 | relpos += step;
> | ~~~^~~
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
> function ‘WinGetFuncArgInFrame’:
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3531:10:
> warning: ‘step’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 3531 | relpos += step;
> | ~~~^~~
>
>
>
> На пт, 8.01.2021 г. в 2:02 ч. Vik Fearing
> написа:
>
>> On 1/1/21 10:21 PM, Zhihong Yu wrote:
>> > Krasiyan:
>> > Happy New Year.
>> >
>> > For WinGetFuncArgInPartition():
>> >
>> > + if (target > 0)
>> > + step = 1;
>> > + else if (target < 0)
>> > + step = -1;
>> > + else
>> > + step = 0;
>> >
>> > When would the last else statement execute ? Since the above code is
>> > for WINDOW_SEEK_CURRENT, I wonder why step should be 0.
>>
>> Hi.
>>
>> "lag(expr, 0) over w" is useless but valid.
>> --
>> Vik Fearing
>>
>
Fix \watch if expanded output is on and there are no results
Hi,
in expanded output, psql does not print the title if there are zero
results. In regular output, it prints the title ("foo") no matter how
many rows:
postgres=# \pset title foo
Title is "foo".
postgres=# SELECT 1 WHERE 1=1;
foo
?column?
--
1
(1 row)
postgres=# SELECT 1 WHERE 1=2;
foo
?column?
--
(0 rows)
In expanded mode, it only prints the title if there is at least one row:
postgres=# \x
Expanded display is on.
postgres=# SELECT 1 WHERE 1=1;
foo
-[ RECORD 1 ]
?column? | 1
postgres=# SELECT 1 WHERE 1=2;
(0 rows)
Is that intentional? It breaks \watch for expanded output if the query
being watched does not always return at least one row. If that is not
the case, the timestamp is ommitted. I think that's bad enough to
warrant fixing like in the attached.
But maybe there was a good reason the title wasn't printed? On the ohter
hand, we're just returning early from print_aligned_vertical() right now
and never get to the part where we usually print the title if there are
zero results.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.ba...@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index e8772a278c..9f16fed49d 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -1247,6 +1247,10 @@ print_aligned_vertical(const printTableContent *cont,
{
printTableFooter *f;
+ /* print title */
+ if (cont->title)
+fprintf(fout, "%s\n", cont->title);
+
for (f = footers; f; f = f->next)
fprintf(fout, "%s\n", f->data);
}
Re: [HACKERS] [PATCH] Generic type subscripting
On Thu Jan 7, 2021 at 3:24 AM EST, Pavel Stehule wrote:
> čt 7. 1. 2021 v 9:15 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
> > > On Wed, Jan 06, 2021 at 09:22:53PM +0100, Pavel Stehule wrote:
> > >
> > > this case should to raise exception - the value should be changed or
> > error
> > > should be raised
> > >
> > > postgres=# insert into foo values('{}');
> > > postgres=# update foo set a['a'] = '100';
> > > postgres=# update foo set a['a'][1] = '-1';
> > > postgres=# select * from foo;
> > > ┌┐
> > > │ a │
> > > ╞╡
> > > │ {"a": 100} │
> > > └┘
> >
> > I was expecting this question, as I've left this like that intentionally
> > because of two reasons:
> >
> > * Opposite to other changes, to implement this one we need to introduce
> > a condition more interfering with normal processing, which raises
> > performance issues for already existing functionality in jsonb_set.
> >
> > * I vaguely recall there was a similar discussion about jsonb_set with
> > the similar solution.
> >
>
> ok.
>
> In this case I have a strong opinion so current behavior is wrong. It
> can
> mask errors. There are two correct possibilities
>
> 1. raising error - because the update try to apply index on scalar value
>
> 2. replace by array - a = {NULL, -1}
>
> But isn't possible ignore assignment
>
> What do people think about it?
I've been following this thread looking forward to the feature and was
all set to come in on the side of raising an exception here, but then I
tried it in a JS REPL:
; a = {}
{}
; a['a'] = '100'
'100'
; a['a'][1] = -1
-1
; a
{ a: '100' }
; b = {}
{}
; b['b'] = 100
100
; b['b'][1] = -1
-1
; b
{ b: 100 }
Even when the value shouldn't be subscriptable _at all_, the invalid
assignment is ignored silently. But since the patch follows some of
JavaScript's more idiosyncratic leads in other respects (e.g. padding
out arrays with nulls when something is inserted at a higher subscript),
the current behavior makes at least as much sense as JavaScript's
canonical behavior.
There's also the bulk update case to think about. An error makes sense
when there's only one tuple being affected at a time, but with 1000
tuples, should a few no-ops where the JSON turns out to be a structural
mismatch stop the rest from changing correctly? What's the alternative?
The only answer I've got is double-checking the structure in the WHERE
clause, which seems like a lot of effort to go to for something that's
supposed to make working with JSON easier.
Changing the surrounding structure (e.g. turning a['a'] into an array)
seems much more surprising than the no-op, and more likely to have
unforeseen consequences in client code working with the JSON. Ignoring
invalid assignments -- like JavaScript itself -- seems like the best
solution to me.
Use pg_pwrite() in pg_test_fsync
Hi,
Since pg_test_fsync is supposed to simulate some aspects of
PostgreSQL's wal_sync_method settings, I think it should be updated to
use the same system calls (which changed in v12). That's mostly on
principle, though in practice, on one system I've managed to see a
small measurable difference.
I left the fsync-after-closing and non-sync'd tests using write(),
because they weren't using lseek(). The latter case is arguably a bit
odd because it's not overwriting pre-allocated blocks, unlike the
earlier tests.
From 0229b1341afaef812232f6f031ee7ffb5de76080 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Sat, 9 Jan 2021 23:37:18 +1300
Subject: [PATCH] Use pg_pwrite() in pg_test_fsync.
For consistency with the PostgreSQL behavior we're tring to simulate,
use pwrite() instead of lseek() + write().
---
src/bin/pg_test_fsync/pg_test_fsync.c | 43 +++
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 3eddd983c6..693a9478ad 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -290,10 +290,11 @@ test_sync(int writes_per_op)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < writes_per_op; writes++)
-if (write(tmpfile, buf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+if (pg_pwrite(tmpfile,
+ buf,
+ XLOG_BLCKSZ,
+ writes * XLOG_BLCKSZ) != XLOG_BLCKSZ)
die("write failed");
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
-die("seek failed");
}
STOP_TIMER;
close(tmpfile);
@@ -315,11 +316,12 @@ test_sync(int writes_per_op)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < writes_per_op; writes++)
- if (write(tmpfile, buf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (pg_pwrite(tmpfile,
+ buf,
+ XLOG_BLCKSZ,
+ writes * XLOG_BLCKSZ) != XLOG_BLCKSZ)
die("write failed");
fdatasync(tmpfile);
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
- die("seek failed");
}
STOP_TIMER;
close(tmpfile);
@@ -339,12 +341,13 @@ test_sync(int writes_per_op)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < writes_per_op; writes++)
- if (write(tmpfile, buf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (pg_pwrite(tmpfile,
+ buf,
+ XLOG_BLCKSZ,
+ writes * XLOG_BLCKSZ) != XLOG_BLCKSZ)
die("write failed");
if (fsync(tmpfile) != 0)
die("fsync failed");
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
- die("seek failed");
}
STOP_TIMER;
close(tmpfile);
@@ -362,12 +365,13 @@ test_sync(int writes_per_op)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < writes_per_op; writes++)
- if (write(tmpfile, buf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (pg_pwrite(tmpfile,
+ buf,
+ XLOG_BLCKSZ,
+ writes * XLOG_BLCKSZ) != XLOG_BLCKSZ)
die("write failed");
if (pg_fsync_writethrough(tmpfile) != 0)
die("fsync failed");
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
- die("seek failed");
}
STOP_TIMER;
close(tmpfile);
@@ -393,8 +397,10 @@ test_sync(int writes_per_op)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < writes_per_op; writes++)
-if (write(tmpfile, buf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
-
+if (pg_pwrite(tmpfile,
+ buf,
+ XLOG_BLCKSZ,
+ writes * XLOG_BLCKSZ) != XLOG_BLCKSZ)
/*
* This can generate write failures if the filesystem has
* a large block size, e.g. 4k, and there is no support
@@ -402,8 +408,6 @@ test_sync(int writes_per_op)
* size, e.g. XFS.
*/
die("write failed");
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
-die("seek failed");
}
STOP_TIMER;
close(tmpfile);
@@ -457,11 +461,12 @@ test_open_sync(const char *msg, int writes_size)
for (ops = 0; alarm_triggered == false; ops++)
{
for (writes = 0; writes < 16 / writes_size; writes++)
-if (write(tmpfile, buf, writes_size * 1024) !=
+if (pg_pwrite(tmpfile,
+ buf,
+ writes_size * 1024,
+ writes * writes_size * 1024) !=
writes_size * 1024)
die("write failed");
- if (lseek(tmpfile, 0, SEEK_SET) == -1)
-die("seek failed");
}
STOP_TIMER;
close(tmpfile);
--
2.20.1
Re: libpq compression
On Thu, Dec 17, 2020 at 05:54:28PM +0300, Konstantin Knizhnik wrote:
> I am maintaining this code in
> g...@github.com:postgrespro/libpq_compression.git repository.
> I will be pleased if anybody, who wants to suggest any bug
> fixes/improvements of libpq compression, create pull requests: it will be
> much easier for me to merge them.
Thanks for working on this.
I have a patch for zstd compression in pg_dump so I looked at your patch.
I'm attaching some language fixes.
> +zstd_create(int level, zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg,
> char* rx_data, size_t rx_data_size)
> +zlib_create(int level, zpq_tx_func tx_func, zpq_rx_func rx_func, void *arg,
> char* rx_data, size_t rx_data_size)
> +build_compressors_list(PGconn *conn, char** client_compressors, bool
> build_descriptors)
Are you able to run pg_indent to fix all the places where "*" is before the
space ? (And similar style issues).
There are several compression patches in the commitfest, I'm not sure how much
they need to be coordinated, but for sure we should coordinate the list of
compressions available at compile time.
Maybe there should be a central structure for this, or maybe just the ID
numbers of compressors should be a common enum/define. In my patch, I have:
+struct compressLibs {
+ const CompressionAlgorithm alg;
+ const char *name; /* Name in -Z alg= */
+ const char *suffix;/* file extension */
+ const int defaultlevel; /* Default compression level */
+};
Maybe we'd also want to store the "magic number" of each compression library.
Maybe there'd also be a common parsing of compression options.
You're supporting a syntax like zlib,zstd:5, but zstd also supports long-range,
checksum, and rsyncable modes (rsyncable is relevant to pg_dump, but not to
libpq).
I think your patch has an issue here. You have this:
src/interfaces/libpq/fe-connect.c
+pqGetc(&resp, conn);
+index = resp;
+if (index == (char)-1)
+{
+appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext(
+ "server is not
supported requested compression algorithms %s\n"),
+ conn->compression);
+goto error_return;
+}
+Assert(!conn->zstream);
+conn->zstream = zpq_create(conn->compressors[index].impl,
+
conn->compressors[index].level,
+
(zpq_tx_func)pqsecure_write, (zpq_rx_func)pqsecure_read, conn,
+
&conn->inBuffer[conn->inCursor], conn->inEnd-conn->inCursor);
This takes the "index" returned by the server and then accesses
conn->compressors[index] without first checking if the index is out of range,
so a malicious server could (at least) crash the client by returning index=666.
I suggest that there should be an enum of algorithms, which is constant across
all servers. They would be unconditionally included and not #ifdef depending
on compilation options.
That would affect the ZpqAlgorithm data structure, which would include an ID
number similar to
src/bin/pg_dump/compress_io.h:typedef enum...CompressionAlgorithm;
The CompressionAck would send the ID rather than the "index".
A protocol analyzer like wireshark could show "Compression: Zstd".
You'd have to verify that the ID is supported (and not bogus).
Right now, when I try to connect to an unpatched server, I get:
psql: error: expected authentication request from server, but received v
+/*
+ * Array with all supported compression algorithms.
+ */
+static ZpqAlgorithm const zpq_algorithms[] =
+{
+#if HAVE_LIBZSTD
+ {zstd_name, zstd_create, zstd_read, zstd_write, zstd_free, zstd_error,
zstd_buffered_tx, zstd_buffered_rx},
+#endif
+#if HAVE_LIBZ
+ {zlib_name, zlib_create, zlib_read, zlib_write, zlib_free, zlib_error,
zlib_buffered_tx, zlib_buffered_rx},
+#endif
+ {no_compression_name}
+};
In config.sgml, it says that libpq_compression defaults to on (on the server
side), but in libpq.sgml it says that it defaults to off (on the client side).
Is that what's intended ? I would've thought the defaults would match, or that
the server would enforce a default more conservative than the client's (the DBA
should probably have to explicitly enable compression, and need to "opt-in"
rather than "opt-out").
Maybe instead of a boolean, this should be a list of permitted compression
algorithms. This allows the admin to set a "policy" rather than using the
server's hard-coded preferences. This could be important to disable an
algorithm at run-time if there's a vulnerability found, or performance problem,
or buggy client, or for diagnostics, or
Re: [HACKERS] [PATCH] Generic type subscripting
so 9. 1. 2021 v 21:06 odesílatel Dian M Fay napsal:
> On Thu Jan 7, 2021 at 3:24 AM EST, Pavel Stehule wrote:
> > čt 7. 1. 2021 v 9:15 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> > napsal:
> >
> > > > On Wed, Jan 06, 2021 at 09:22:53PM +0100, Pavel Stehule wrote:
> > > >
> > > > this case should to raise exception - the value should be changed or
> > > error
> > > > should be raised
> > > >
> > > > postgres=# insert into foo values('{}');
> > > > postgres=# update foo set a['a'] = '100';
> > > > postgres=# update foo set a['a'][1] = '-1';
> > > > postgres=# select * from foo;
> > > > ┌┐
> > > > │ a │
> > > > ╞╡
> > > > │ {"a": 100} │
> > > > └┘
> > >
> > > I was expecting this question, as I've left this like that
> intentionally
> > > because of two reasons:
> > >
> > > * Opposite to other changes, to implement this one we need to introduce
> > > a condition more interfering with normal processing, which raises
> > > performance issues for already existing functionality in jsonb_set.
> > >
> > > * I vaguely recall there was a similar discussion about jsonb_set with
> > > the similar solution.
> > >
> >
> > ok.
> >
> > In this case I have a strong opinion so current behavior is wrong. It
> > can
> > mask errors. There are two correct possibilities
> >
> > 1. raising error - because the update try to apply index on scalar value
> >
> > 2. replace by array - a = {NULL, -1}
> >
> > But isn't possible ignore assignment
> >
> > What do people think about it?
>
> I've been following this thread looking forward to the feature and was
> all set to come in on the side of raising an exception here, but then I
> tried it in a JS REPL:
>
> ; a = {}
> {}
> ; a['a'] = '100'
> '100'
> ; a['a'][1] = -1
> -1
> ; a
> { a: '100' }
>
> ; b = {}
> {}
> ; b['b'] = 100
> 100
> ; b['b'][1] = -1
> -1
> ; b
> { b: 100 }
>
> Even when the value shouldn't be subscriptable _at all_, the invalid
> assignment is ignored silently. But since the patch follows some of
> JavaScript's more idiosyncratic leads in other respects (e.g. padding
> out arrays with nulls when something is inserted at a higher subscript),
> the current behavior makes at least as much sense as JavaScript's
> canonical behavior.
>
> There's also the bulk update case to think about. An error makes sense
> when there's only one tuple being affected at a time, but with 1000
> tuples, should a few no-ops where the JSON turns out to be a structural
> mismatch stop the rest from changing correctly? What's the alternative?
> The only answer I've got is double-checking the structure in the WHERE
> clause, which seems like a lot of effort to go to for something that's
> supposed to make working with JSON easier.
>
> Changing the surrounding structure (e.g. turning a['a'] into an array)
> seems much more surprising than the no-op, and more likely to have
> unforeseen consequences in client code working with the JSON. Ignoring
> invalid assignments -- like JavaScript itself -- seems like the best
> solution to me.
>
We don't need 100% compatibility in possible buggy behaviour.
I very much disliked the situation when the system reports ok, but the
operation was ignored. It is pretty hard to identify bugs in this system.
What can be the benefit or use case for this behavior? JavaScript is
designed for use in web browsers - and a lot of technologies there are
fault tolerant. But this is a database. I would like to know about all the
errors there.
Re: [HACKERS] [PATCH] Generic type subscripting
On Sat Jan 9, 2021 at 3:34 PM EST, Pavel Stehule wrote:
> so 9. 1. 2021 v 21:06 odesílatel Dian M Fay
> napsal:
>
> > On Thu Jan 7, 2021 at 3:24 AM EST, Pavel Stehule wrote:
> > > čt 7. 1. 2021 v 9:15 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> > > napsal:
> > >
> > > > > On Wed, Jan 06, 2021 at 09:22:53PM +0100, Pavel Stehule wrote:
> > > > >
> > > > > this case should to raise exception - the value should be changed or
> > > > error
> > > > > should be raised
> > > > >
> > > > > postgres=# insert into foo values('{}');
> > > > > postgres=# update foo set a['a'] = '100';
> > > > > postgres=# update foo set a['a'][1] = '-1';
> > > > > postgres=# select * from foo;
> > > > > ┌┐
> > > > > │ a │
> > > > > ╞╡
> > > > > │ {"a": 100} │
> > > > > └┘
> > > >
> > > > I was expecting this question, as I've left this like that
> > intentionally
> > > > because of two reasons:
> > > >
> > > > * Opposite to other changes, to implement this one we need to introduce
> > > > a condition more interfering with normal processing, which raises
> > > > performance issues for already existing functionality in jsonb_set.
> > > >
> > > > * I vaguely recall there was a similar discussion about jsonb_set with
> > > > the similar solution.
> > > >
> > >
> > > ok.
> > >
> > > In this case I have a strong opinion so current behavior is wrong. It
> > > can
> > > mask errors. There are two correct possibilities
> > >
> > > 1. raising error - because the update try to apply index on scalar value
> > >
> > > 2. replace by array - a = {NULL, -1}
> > >
> > > But isn't possible ignore assignment
> > >
> > > What do people think about it?
> >
> > I've been following this thread looking forward to the feature and was
> > all set to come in on the side of raising an exception here, but then I
> > tried it in a JS REPL:
> >
> > ; a = {}
> > {}
> > ; a['a'] = '100'
> > '100'
> > ; a['a'][1] = -1
> > -1
> > ; a
> > { a: '100' }
> >
> > ; b = {}
> > {}
> > ; b['b'] = 100
> > 100
> > ; b['b'][1] = -1
> > -1
> > ; b
> > { b: 100 }
> >
> > Even when the value shouldn't be subscriptable _at all_, the invalid
> > assignment is ignored silently. But since the patch follows some of
> > JavaScript's more idiosyncratic leads in other respects (e.g. padding
> > out arrays with nulls when something is inserted at a higher subscript),
> > the current behavior makes at least as much sense as JavaScript's
> > canonical behavior.
> >
> > There's also the bulk update case to think about. An error makes sense
> > when there's only one tuple being affected at a time, but with 1000
> > tuples, should a few no-ops where the JSON turns out to be a structural
> > mismatch stop the rest from changing correctly? What's the alternative?
> > The only answer I've got is double-checking the structure in the WHERE
> > clause, which seems like a lot of effort to go to for something that's
> > supposed to make working with JSON easier.
> >
> > Changing the surrounding structure (e.g. turning a['a'] into an array)
> > seems much more surprising than the no-op, and more likely to have
> > unforeseen consequences in client code working with the JSON. Ignoring
> > invalid assignments -- like JavaScript itself -- seems like the best
> > solution to me.
> >
>
> We don't need 100% compatibility in possible buggy behaviour.
>
> I very much disliked the situation when the system reports ok, but the
> operation was ignored. It is pretty hard to identify bugs in this
> system.
>
> What can be the benefit or use case for this behavior? JavaScript is
> designed for use in web browsers - and a lot of technologies there are
> fault tolerant. But this is a database. I would like to know about all
> the
> errors there.
I'm thinking of the update path as a kind of implicit schema. JSON is
intentionally not bound to any schema on creation, so I don't see a
failure to enforce another schema at runtime (and outside the WHERE
clause, at that) as an error exactly.
But I looked into the bulk case a little further, and "outside the
WHERE clause" cuts both ways. The server reports an update whether or
not the JSON could have been modified, which suggests triggers will
fire for no-op updates. That's more clearly a problem.
insert into j (val) values
('{"a": 100}'),
('{"a": "200"}'),
('{"b": "300"}'),
('{"c": {"d": 400}}'),
('{"a": {"z": 500}}');
INSERT 0 5
update j set val['a']['z'] = '600' returning *;
val
{"a": 100}
{"a": "200"}
{"a": {"z": 600}, "b": "300"}
{"a": {"z": 600}, "c": {"d": 400}}
{"a": {"z": 600}}
(5 rows)
*UPDATE 5*
Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
On Sat, Jan 09, 2021 at 08:25:39PM +0500, Andrey Borodin wrote: > > 9 янв. 2021 г., в 15:17, Noah Misch написал(а): > >> I'm a little bit afraid that this kind of patch can hide bugs (while > >> potentially saving some users data). Besides this patch seems like a > >> useful precaution. Maybe we could emit scary warnings if SLRU segments do > >> not stack into continuous range? > > > > Scary warnings are good for an observation that implies a bug, but the > > slru-truncate-t-insurance patch causes such an outcome in non-bug cases > > where > > it doesn't happen today. In other words, discontinuous ranges of SLRU > > segments would be even more common after that patch. For example, it would > > happen anytime oldestXID advances by more than ~1B at a time. > > Uhm, I thought that if there is going to be more than ~1B xids - we are going > to keep all segements forever and range still will be continuous. Or am I > missing something? No; it deletes the most recent ~1B and leaves the older segments. An exception is multixact, as described in the commit message and the patch's change to a comment in TruncateMultiXact().
Re: support for MERGE
On 1/8/21 8:22 PM, Alvaro Herrera wrote:
> On 2020-Dec-31, Alvaro Herrera wrote:
>
>> Here's a rebase of Simon/Pavan's MERGE patch to current sources. I
>> cleaned up some minor things in it, but aside from rebasing, it's pretty
>> much their work (even the commit message is Simon's).
>
> Rebased, no changes.
>
I took a look at this today. Some initial comments (perhaps nitpicking,
in some cases).
1) sgml docs
This probably needs more work. Some of the sentences (in mvcc.sgml) are
so long I can't quite parse them. Maybe that's because I'm not a native
speaker, but still ... :-/
Also, there are tags missing - UPDATE/INSERT/... should be or
maybe , depending on the context. I think the new docs are a
bit confused which of those it should be, but I'd say should
be used for SQL commands and for MERGE actions?
It'd be a mess to list all the places, so the attached patch (0001)
tweaks this. Feel free to reject changes that you disagree with.
The patch also adds a bunch of XXX comments, suggesting some changes
(clarifications, removing unnecessarily duplicate text, etc.)
2) explain.c
I'm a bit confused about this
case CMD_MERGE:
operation = "Merge";
foperation = "Foreign Merge";
break;
because the commit message says foreign tables are not supported. So why
do we need this?
3) nodeModifyTable.c
ExecModifyTable does this:
if (operation == CMD_MERGE)
{
ExecMerge(node, resultRelInfo, estate, slot, junkfilter);
continue;
}
i.e. it handles the MERGE far from the other DML operations. That seems
somewhat strange, especially without any comment - can't we refactor
this and handle it in the switch with the other DML?
4) preptlist.c
I propose to add a brief comment in preprocess_targetlist, explaining
what we need to do for CMD_MERGE (see 0001). And I think it'd be good to
explain why MERGE uses the same code as UPDATE (it's not obvious to me).
5) WHEN AND
I admit the "WHEN AND" conditions sounds a bit cryptic - it took me a
while to realize what this refers to. Is that a term established by SQL
Standard, or something we invented?
6) walsender.c
Huh, why does this patch touch this at all?
7) rewriteHandler.c
I see MERGE "doesn't support" rewrite rules in the sense that it simply
ignores them. Shouldn't it error-out instead? Seems like a foot-gun to
me, because people won't realize this limitation and may not notice
their rules don't fire.
8) varlena.c
Again, why are these changes to length checks in a MERGE patch?
9) parsenodes.h
Should we rename mergeTarget_relation to mergeTargetRelation? The
current name seems like a mix between two naming schemes.
10) per valgrind, there's a bug in ExecDelete
The table_tuple_delete may not set tmfd (which is no stack), leaving it
not initialized. But the code later branches depending on this. The 0002
patch fixes that by simply setting it to OK before the call, which makes
the valgrind error go away. But maybe it should be fixed in a different
way (e.g. by setting it at the beginning of table_tuple_delete).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
>From a332b1c279e847ec533ff34ffc35303672cd Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sat, 9 Jan 2021 03:17:56 +0100
Subject: [PATCH 1/2] review
---
doc/src/sgml/mvcc.sgml | 20 +++-
doc/src/sgml/ref/create_policy.sgml| 6 +++---
doc/src/sgml/ref/merge.sgml| 12 +---
src/backend/commands/explain.c | 2 +-
src/backend/executor/execMerge.c | 5 -
src/backend/executor/nodeModifyTable.c | 3 ++-
src/backend/optimizer/prep/preptlist.c | 6 ++
src/backend/parser/parse_agg.c | 1 +
src/backend/replication/walsender.c| 4 ++--
src/backend/rewrite/rewriteHandler.c | 4
src/backend/utils/adt/varlena.c| 3 +++
src/include/nodes/parsenodes.h | 7 ---
12 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index 9ec8474185..02c9f3fdea 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -429,22 +429,24 @@ COMMIT;
with both INSERT and UPDATE
subcommands looks similar to INSERT with an
ON CONFLICT DO UPDATE clause but does not guarantee
-that either INSERT and UPDATE will
occur.
+that either INSERT or UPDATE will
occur.
-If MERGE attempts an UPDATE or DELETE and the row is concurrently updated
+ /* XXX This is a very long and hard to understand sentence :-( */
+ /* XXX Do we want to mention EvalPlanQual here? There's no explanation
what it does in this file, so maybe elaborate what it does or leave that detail
for a README? */
+If MERGE attempts an UPDATE or
DELETE and the row is concurrently updated
but the join condition still passes for the current target and the current
-source tuple, then MERGE will behave the same as the U
Re: Key management with tests
On Sat, Jan 9, 2021 at 08:08:16PM -0500, Bruce Momjian wrote: > On Sat, Jan 9, 2021 at 01:17:36PM -0500, Bruce Momjian wrote: > > I know we are still working on the hex patch (dest-len) and the crypto > > tests, but I wanted to post this so people can see where we are, and we > > can get some current cfbot testing. > > Here is an updated version that covers all the possible > testing/configuration options. Does anyone know why the cfbot applied the patch listed second first here? http://cfbot.cputube.org/patch_31_2925.log Specifically, it applied hex..key.diff.gz before hex.diff.gz. I assumed it would apply attachments in the order they appear in the email. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Re: Key management with tests
On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian wrote: > Does anyone know why the cfbot applied the patch listed second first > here? > > http://cfbot.cputube.org/patch_31_2925.log > > Specifically, it applied hex..key.diff.gz before hex.diff.gz. I assumed > it would apply attachments in the order they appear in the email. It sorts the filenames (in this case after decompressing step removes the .gz endings). That works pretty well for the patches that "git format-patch" spits out, but it's a bit hit and miss with cases like yours.
Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding
> 10 янв. 2021 г., в 03:15, Noah Misch написал(а): > > No; it deletes the most recent ~1B and leaves the older segments. An > exception is multixact, as described in the commit message and the patch's > change to a comment in TruncateMultiXact(). Thanks for clarification. One more thing: retention point at 3/4 of overall space (half of wraparound) seems more or less random to me. Why not 5/8 or 9/16? Can you please send revised patches with fixes? Thanks! Best regards, Andrey Borodin.
