Re: Error on failed COMMIT
On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov wrote: > Noone suggested that "commit leaves the session in a transaction state". > Of course, every commit should terminate the transaction. > However, if a commit fails (for any reason), it should produce the relevant > ERROR that explains what went wrong rather than silently doing a rollback. OK, I guess I misinterpreted the proposal. That would be much less problematic -- any driver or application that can't handle ERROR in response to an attempted COMMIT would be broken already. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
RE: [Proposal] Add accumulated statistics for wait event
On Fri, Feb 14, 2020 at 11:59 AM, 王胜利 wrote: >I am glad to know you are working on PG accumulated statistics > feature, and I am interested on it. >I see these two patch file you made, can you let me know which branch > of PG code based? > >when I use this: https://github.com/postgres/postgres/commits/master, > and apply these patches, report some error. Thanks for Wang's mail, I noticed my 0002 patch was wrong from v3. Here, I attach correct patches. Also I will begin to do some benchmark with higher scale and higher number of users and try to change stats reporting implementation to not affect performance, which I couldn't have not started because of another tasks. -- Yoshikazu Imai 0001-Add-pg_stat_waitaccum-view-v6.patch Description: 0001-Add-pg_stat_waitaccum-view-v6.patch 0002-POC-Change-measuring-method-of-wait-event-time-fr-v6.patch Description: 0002-POC-Change-measuring-method-of-wait-event-time-fr-v6.patch
Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
On 2020-02-24 12:21, Kuntal Ghosh wrote:
I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.
diff --git a/contrib/test_decoding/sql/toast.sql
b/contrib/test_decoding/sql/toast.sql
..
-INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 1));
-SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
+INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
+SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?
OK, this is interesting. The details of this are somewhat unfamiliar to
me, but it appears that due to TOAST_INDEX_HACK in indextuple.c, an
index tuple cannot be larger than 8191 bytes when untoasted (but not
uncompressed).
What the test case above is testing is a situation where the heap tuple
is stored toasted uncompressed (storage external) but the index tuple is
not (probably compressed inline). This is exactly the situation that I
was contending should not be possible, because it cannot be dumped or
restored.
An alternative would be that we make this situation fully supported.
Then we'd probably need at least ALTER INDEX ... ALTER COLUMN ... SET
STORAGE, and some pg_dump support.
Thoughts?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RS_EPHEMERAL vs RS_TEMPORARY
I'm trying to figure out what's specific about RS_EPHEMERAL and RS_TEMPORARY slot kinds. The following comment (see definition of the ReplicationSlotPersistency enumeration) tells when each kind is dropped * Slots marked as PERSISTENT are crash-safe and will not be dropped when * released. Slots marked as EPHEMERAL will be dropped when released or after * restarts. Slots marked TEMPORARY will be dropped at the end of a session * or on error. ... typedef enum ReplicationSlotPersistency However I don't see the actual difference: whenever ReplicationSlotCleanup() is called (on error or session end), ReplicationSlotRelease() has already been called too. And as for server restart, I see that RestoreSlotFromDisk() discards both EPHEMERAL and TEMPORARY. Do we really need both of them? -- Antonin Houska Web: https://www.cybertec-postgresql.com
Re: Error on failed COMMIT
Robert>Now, of course, it's also true that if what the server does makes Robert>users sad, maybe the server should do something different The server makes users sad as it reports the same end result (=="commit failed") differently. Sometimes the server produces ERROR, and sometimes the server produces "OK, the transaction was rolled back". The users do expect that commit might fail, and they don't really expect that sometimes commit can be silently converted to a rollback. Robert>BEGIN; Robert>-- do stuff Robert>COMMIT; Robert>BEGIN; Robert>-- do more stuff Robert>COMMIT; Robert>...and they run these scripts by piping them into psql. Now, if the Robert>COMMIT leaves the session in a transaction state, Noone suggested that "commit leaves the session in a transaction state". Of course, every commit should terminate the transaction. However, if a commit fails (for any reason), it should produce the relevant ERROR that explains what went wrong rather than silently doing a rollback. Vladimir
Re: plan cache overhead on plpgsql expression
Hi I added this patch to a commitfest https://commitfest.postgresql.org/27/2467/ It is very interesting speedup and it is in good direction to JIT expressions Pavel
Re: False failure during repeated windows build.
At Tue, 25 Feb 2020 14:02:04 +0900, Michael Paquier wrote in > On Tue, Feb 25, 2020 at 10:14:10AM +0900, Kyotaro Horiguchi wrote: > > At Fri, 21 Feb 2020 14:02:40 +0100, Juan José Santamaría Flecha > > wrote in > > > After commit 9573384 this patch no longer applies, but with a trivial > > > rebase it fixes the issue. > > > > Thanks! This is the rebased version. I'll register this to the next CF. > > That's annoying, and you are right. So, committed. Thank you for committing. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 6:40 PM Dave Cramer wrote: > Fair enough. What I meant to say was that the driver isn't in the business of > providing different semantics than the server provides. Still don't agree. The server doesn't make any decision about what semantics the driver has to provide. The driver can do whatever it wants. If what it does makes users sad, then maybe it ought to do something different. Now, of course, it's also true that if what the server does makes users sad, maybe the server should do something different. But I think you're vastly underestimating the likely impact on other users and drivers of making this change. That is a guess, and like any guess, may be wrong. But it is still what I think. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Error on failed COMMIT
>do you think most common connection poolers would continue to >work after making this change? Of course, they should. There are existing cases when commit responds with an error: deferrable constraints. There's nothing new except it is suggested to make the behavior of commit/prepare failure (e.g. "can't commit the transaction because...") consistent with other commit failures (e.g. deferred violation). Vladimir
Re: PG v12.2 - Setting jit_above_cost is causing the server to crash
On Mon, Feb 24, 2020 at 8:20 PM Tom Lane wrote: > Aditya Toshniwal writes: > > On Mon, Feb 24, 2020 at 12:46 PM Andres Freund > wrote: > >> This isn't reproducible here. Are you sure that you're running on a > >> clean installation? > > > Yes I did a fresh installation using installer provided here - > > https://www.enterprisedb.com/downloads/postgresql > > There is apparently something wrong with the JIT stuff in EDB's 12.2 > build for macOS. At least, that's the conclusion I came to after > off-list discussion with the submitter of bug #16264, which has pretty > much exactly this symptom (especially if you're seeing "signal 9" > reports in the postmaster log). For him, either disabling JIT or > reverting to 12.1 made it go away. > Yes it seems like issue with EDB build. It works fine on macOS < Catalina. > > regards, tom lane > -- Thanks and Regards, Aditya Toshniwal pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE"
Re: False failure during repeated windows build.
On Tue, Feb 25, 2020 at 10:14:10AM +0900, Kyotaro Horiguchi wrote: > At Fri, 21 Feb 2020 14:02:40 +0100, Juan José Santamaría Flecha > wrote in > > After commit 9573384 this patch no longer applies, but with a trivial > > rebase it fixes the issue. > > Thanks! This is the rebased version. I'll register this to the next CF. That's annoying, and you are right. So, committed. -- Michael signature.asc Description: PGP signature
Re: Function to track shmem reinit time
On Sat, Feb 22, 2020 at 10:31 PM Tom Lane wrote: > I'm still going to object to it, on the grounds that > > (1) it's exposing an implementation detail that clients should not be > concerned with, and that we might change in future. The name isn't > even well chosen --- if the argument is that it's useful to monitor > server uptime, why isn't the name "pg_server_uptime"? > > (2) if your server is crashing often enough that postmaster start > time isn't an adequate substitute, you have way worse problems than > whether you can measure it. I'd rather see us put effort into > fixing whatever the underlying problem is. I don't accept the first argument, because I think the chances that we'll change our basic approach to this problem are indistinguishable from zero. A few years back, you criticized some idea of mine as tantamount to rm -rf src/backend/optimizer, which you were not ready to endorse. That proposal was surely vastly less ambitious than some proposal which would remove the idea of shared memory reinitialization after an unsafe backend exit. I don't accept the second argument either, because it's a false dichotomy. Adding this function won't reduce the amount of energy that gets spent fixing crashes. There are always more crashes. I realize that several other people voted against this proposal so I don't think it's OK to commit a patch in this area unless some more positive votes turn up, but I'm still +1 on the idea. Granted, we don't want an infinite amount of clutter in the system catalogs, but I have a hard time believing that this proposed function would get any less use than the hyperbolic trig functions. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Unicode escapes with any backend encoding
On Mon, Feb 24, 2020 at 11:19 PM Tom Lane wrote: > I see this patch got sideswiped by the recent refactoring of JSON > lexing. Here's an attempt at fixing it up. Since the frontend > code isn't going to have access to encoding conversion facilities, > this creates a difference between frontend and backend handling > of JSON Unicode escapes, which is mildly annoying but probably > isn't going to bother anyone in the real world. Outside of > jsonapi.c, there are no changes from v2. For the record, as far as JSON goes, I think I'm responsible for the current set of restrictions, and I'm not attached to them. I believe I was uncertain of my ability to implement anything better than what we have now and also slightly unclear on what the semantics ought to be. I'm happy to see it improved, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: explain HashAggregate to report bucket and memory stats
On Sat, Feb 22, 2020 at 10:53:35PM +0100, Tomas Vondra wrote:
> 1) explain.c API
>
> The functions in explain.c (even the static ones) follow the convention
> that the last parameter is (ExplainState *es). I think we should stick
> to this, so the new parameters should be added before it.
I found it weird to have the "constant" arguments at the end rather than at the
beginning. (Also, these don't follow that convention: show_buffer_usage
ExplainSaveGroup ExplainRestoreGroup ExplainOneQuery ExplainPrintJIT).
But done.
> Also, the first show_grouping_sets should be renamed to aggstate to make
> it consistent with the type change.
The prototype wasn't updated - fixed.
> 2) The hash_instrumentation is a bit inconsistent with what we already
> have ..HashTableInstrumentation..
Thanks for thinking of a better name.
> 5) I think the explain for grouping sets need a rething. Teh function
> show_grouping_set_keys was originally meant to print just the keys, but
> now it's also printing the hash table stats. IMO we need a new function
> printing a grouping set info - calling show_grouping_set_keys to print
> the keys, but then also printing the extra hashtable info.
I renamed it, and did the rest in a separate patch for now, since I'm only
partially convinced it's an improvement.
> 6) subplan explain
>
> That is, there's no indication why would this use a hash table, because
> the "hashed subplan" is included only in verbose mode:
Need to think about that..
> Not sure if this is an issue, maybe it's fine. But it's definitely
> strange that we only print memory info in verbose mode - IMHO it's much
> more useful info than the number of buckets etc.
You're right that verbose isn't right for this.
I wrote patches creating new explain options to allow stable output of "explain
analyze", by avoiding Memory/Disk. The only other way to handle it seems to be
to avoid "explain analyze" in regression tests, which is what's in common
practice anyway, so did that instead.
I also fixed wrong output and wrong non-text formatting for grouping sets,
tweaked output for subplan, and broke style rules less often.
--
Justin
>From 4edb6652f8e8923e0ae7f044817a30b9024b3f49 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Tue, 31 Dec 2019 18:49:41 -0600
Subject: [PATCH v5 1/7] explain to show tuplehash bucket and memory stats..
Note that hashed SubPlan and recursiveUnion aren't affected in explain output,
probably since hashtables aren't allocated at that point.
Discussion: https://www.postgresql.org/message-id/flat/20200103161925.gm12...@telsasoft.com
---
src/backend/commands/explain.c| 137 ++--
src/backend/executor/execGrouping.c | 27
src/backend/executor/nodeAgg.c| 11 ++
src/backend/executor/nodeRecursiveunion.c | 3 +
src/backend/executor/nodeSetOp.c | 1 +
src/backend/executor/nodeSubplan.c| 3 +
src/include/executor/executor.h | 1 +
src/include/nodes/execnodes.h | 9 ++
src/test/regress/expected/aggregates.out | 36 --
src/test/regress/expected/groupingsets.out| 64 +++---
src/test/regress/expected/join.out| 3 +-
src/test/regress/expected/matview.out | 9 +-
src/test/regress/expected/partition_aggregate.out | 145 +-
src/test/regress/expected/partition_join.out | 13 +-
src/test/regress/expected/pg_lsn.out | 3 +-
src/test/regress/expected/select_distinct.out | 3 +-
src/test/regress/expected/select_parallel.out | 11 +-
src/test/regress/expected/subselect.out | 3 +-
src/test/regress/expected/tablesample.out | 3 +-
src/test/regress/expected/union.out | 15 ++-
src/test/regress/expected/window.out | 3 +-
src/test/regress/expected/write_parallel.out | 16 ++-
22 files changed, 429 insertions(+), 90 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4..ff22181 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -18,6 +18,7 @@
#include "commands/createas.h"
#include "commands/defrem.h"
#include "commands/prepare.h"
+#include "executor/nodeAgg.h"
#include "executor/nodeHash.h"
#include "foreign/fdwapi.h"
#include "jit/jit.h"
@@ -86,12 +87,14 @@ static void show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
ExplainState *es);
static void show_agg_keys(AggState *astate, List *ancestors,
ExplainState *es);
-static void show_grouping_sets(PlanState *planstate, Agg *agg,
+static void show_grouping_sets(AggState *aggstate, Agg *agg,
List *ancestors, ExplainState *es);
-static void show_grouping_set_keys(PlanState *planstate,
+static void show_grouping_set_info(AggState *aggstate,
Agg *aggnode, Sort *sortnode,
List
Re: client-side fsync() error handling
On Mon, Feb 24, 2020 at 05:03:07PM +0100, Peter Eisentraut wrote: > committed Thanks! -- Michael signature.asc Description: PGP signature
Re: [Patch] Make pg_checksums skip foreign tablespace directories
On Mon, Feb 24, 2020 at 01:11:10PM +0100, Bernd Helmle wrote: > The other makes scan_directories() complicated to read and special > cases just a single directory in an otherwise more or less generic > function. E.g. it makes me uncomfortable if we get a pg_tblspc > somewhere else than PGDATA (if someone managed to create such a > directory in a foreign tablespace location for example), so we should > maintain an additional check if we really operate on the pg_tblspc we > have to. That was the reason(s) i've moved it into a separate function. We are just discussing about the code path involving scanning a directory, so that does not seem that bad to me. I really think that we should avoid duplicating the same logic around, and that we should remain consistent with non-directory entries in those paths, complaining with a proper failure if extra, unwanted files are present. -- Michael signature.asc Description: PGP signature
Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
On Mon, Feb 24, 2020 at 03:29:03PM -0800, Andres Freund wrote: > I do find it a bit odd that you essentially duplicated the existing > comment in a different phrasing. What's the point? A direct reference to catalog tuples is added for each code path calling log_heap_new_cid(), so that was more natural to me. -- Michael signature.asc Description: PGP signature
Re: Internal key management system
On Thu, 20 Feb 2020 at 16:16, Masahiko Sawada
wrote:
>
> On Wed, 19 Feb 2020 at 09:29, Cary Huang wrote:
> >
> > Hi
> >
> > I have tried the attached kms_v3 patch and have some comments:
> >
> > 1) In the comments, I think you meant hmac + iv + encrypted data instead of
> > iv + hmac + encrypted data?
> >
> > ---> in kmgr_wrap_key( ):
> > + /*
> > +* Assemble the wrapped key. The order of the wrapped key is iv,
> > hmac and
> > +* encrypted data.
> > +*/
>
> Right, will fix.
>
> >
> >
> > 2) I see that create_keywrap_ctx function in kmgr_utils.c and regular
> > cipher context init will both call ossl_aes256_encrypt_init to initialise
> > context for encryption and key wrapping. In ossl_aes256_encrypt_init, the
> > cipher method always initialises to aes-256-cbc, which is ok for keywrap
> > because under CBC block cipher mode, we only had to supply one unique IV as
> > initial value. But for actual WAL and buffer encryption that will come in
> > later, I think the discussion is to use CTR block cipher mode, which
> > requires one unique IV for each block, and the sequence id from WAL and
> > buffer can be used to derive unique IV for each block for better security?
> > I think it would be better to allow caller to decide which EVP_CIPHER to
> > initialize? EVP_aes_256_cbc(), EVP_aes_256_ctr() or others?
> >
> > +ossl_aes256_encrypt_init(pg_cipher_ctx *ctx, uint8 *key)
> > +{
> > + if (!EVP_EncryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, NULL, NULL))
> > + return false;
> > + if (!EVP_CIPHER_CTX_set_key_length(ctx, PG_AES256_KEY_LEN))
> > + return false;
> > + if (!EVP_EncryptInit_ex(ctx, NULL, NULL, key, NULL))
> > + return false;
> > +
> > + /*
> > +* Always enable padding. We don't need to check the return
> > +* value as EVP_CIPHER_CTX_set_padding always returns 1.
> > +*/
> > + EVP_CIPHER_CTX_set_padding(ctx, 1);
> > +
> > + return true;
> > +}
>
> It seems good. We can expand it to make caller decide the block cipher
> mode of operation and key length. I removed such code from the
> previous patch to make it simple since currently we support only
> AES-256 CBC.
>
> >
> > 3) Following up point 2), I think we should enhance the enum to include not
> > only the Encryption algorithm and key size, but also the block cipher mode
> > as well because having all 3 pieces of information can describe exactly how
> > KMS is performing the encryption and decryption. So when we call
> > "ossl_aes256_encrypt_init", we can include the new enum as input parameter
> > and it will initialise the EVP_CIPHER_CTX with either EVP_aes_256_cbc() or
> > EVP_aes_256_ctr() for different purposes (key wrapping, or WAL
> > encryption..etc).
> >
> > ---> kmgr.h
> > +/* Value of key_management_cipher */
> > +enum
> > +{
> > + KMGR_CIPHER_OFF = 0,
> > + KMGR_CIPHER_AES256
> > +};
> > +
> >
> > so it would become
> > +enum
> > +{
> > + KMGR_CIPHER_OFF = 0,
> > + KMGR_CIPHER_AES256_CBC = 1,
> > + KMGR_CIPHER_AES256_CTR = 2
> > +};
> >
> > If you agree with this change, several other places will need to be changed
> > as well, such as "kmgr_cipher_string", "kmgr_cipher_value" and the initdb
> > code
>
> KMGR_CIPHER_XXX is relevant with cipher mode used by KMS and KMS would
> still use AES256 CBC even if we had TDE which would use AES256 CTR.
>
> After more thoughts, I think currently we can specify -e aes-256 to
> initdb but actually this is not necessary. When
> --cluster-passphrase-command specified, we enable the internal KMS and
> always use AES256 CBC. Something like -e option will be needed after
> supporting TDE with several cipher options. Thoughts?
>
> >
> > 4) the pg_wrap_key and pg_unwrap_key SQL functions defined in kmgr.c
> > I tried these new SQL functions and found that the pg_unwrap_key will
> > produce the original key with 4 bytes less. This is because the result
> > length is not set long enough to accommodate the 4 byte VARHDRSZ header
> > used by the multi-type variable.
> >
> > the len variable in SET_VARSIZE(res, len) should include also the variable
> > header VARHDRSZ. Now it is 4 byte short so it will produce incomplete
> > output.
> >
> > ---> pg_unwrap_key function in kmgr.c
> > + if (!kmgr_unwrap_key(UnwrapCtx, (uint8 *) VARDATA_ANY(data),
> > datalen,
> > +(uint8 *) VARDATA(res),
> > ))
> > + ereport(ERROR,
> > + (errmsg("could not unwrap the given
> > secret")));
> > +
> > + /*
> > +* The size of unwrapped key can be smaller than the size estimated
> > +* before unwrapping since the padding is removed during unwrapping.
> > +*/
> > + SET_VARSIZE(res, len);
> > + PG_RETURN_BYTEA_P(res);
> >
> > I am only testing their functionalities with random key as input data. It
> > is
Re: pg_trigger.tgparentid
Hi Alvaro, On Tue, Feb 25, 2020 at 3:58 AM Alvaro Herrera wrote: > On 2020-Feb-19, Amit Langote wrote: > > > Or: > > > > * However, if our parent is a partition itself, there might be > > * internal triggers that must not be skipped. For example, > > triggers > > * on the parent that were in turn cloned from its own parent are > > * marked internal, which must be cloned to the partition. > > Thanks for pointing this out -- I agree it needed rewording. I slightly > adjusted your text like this: > > * Internal triggers require careful examination. Ideally, > we don't > * clone them. However, if our parent is itself a partition, > there > * might be internal triggers that must not be skipped; for > example, > * triggers on our parent that are in turn clones from its > parent (our > * grandparent) are marked internal, yet they are to be > cloned. > > Is this okay for you? Thanks. Your revised text looks good, except there is a typo: in turn clones -> in turn cloned Thanks, Amit
Re: False failure during repeated windows build.
At Fri, 21 Feb 2020 14:02:40 +0100, Juan José Santamaría Flecha
wrote in
> After commit 9573384 this patch no longer applies, but with a trivial
> rebase it fixes the issue.
Thanks! This is the rebased version. I'll register this to the next CF.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 70a725f2f8fab8b490106f2625ac821ab7680675 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Tue, 18 Feb 2020 15:29:55 +0900
Subject: [PATCH v2] Fix behavior for repeated build on Windows.
Even after the function GenerateConfigHeader in Solution.pm decided
not to generate a new file, it wrongly checks for the remaining macro
defintions and stops with failure. Fix it by not doing the check if it
skipped file generation.
---
src/tools/msvc/Solution.pm | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 75f916399c..6b4a6eec2a 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -826,13 +826,14 @@ EOF
sub GenerateConfigHeader
{
my ($self, $config_header, $defines, $required) = @_;
- my %defines_copy = %$defines;
my $config_header_in = $config_header . '.in';
if (IsNewer($config_header, $config_header_in) ||
IsNewer($config_header, __FILE__))
{
+ my %defines_copy = %$defines;
+
open(my $i, '<', $config_header_in)
|| confess "Could not open $config_header_in\n";
open(my $o, '>', $config_header)
@@ -871,10 +872,11 @@ sub GenerateConfigHeader
}
close($o);
close($i);
- }
- if ($required && scalar(keys %defines_copy) > 0)
- {
- croak "unused defines: " . join(' ', keys %defines_copy);
+
+ if ($required && scalar(keys %defines_copy) > 0)
+ {
+ croak "unused defines: " . join(' ', keys %defines_copy);
+ }
}
}
--
2.18.2
Re: [HACKERS] WAL logging problem in 9.4.3?
At Sat, 22 Feb 2020 21:12:20 -0800, Noah Misch wrote in > On Fri, Feb 21, 2020 at 04:49:59PM +0900, Kyotaro Horiguchi wrote: > > At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi > > wrote in > > > At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch wrote > > > in > > > > - When reusing an index build, instead of storing the dropped relid in > > > > the > > > > IndexStmt and opening the dropped relcache entry in ATExecAddIndex(), > > > > store > > > > the subid fields in the IndexStmt. This is less code, and I felt > > > > RelationIdGetRelationCache() invited misuse. > > > > > > Hmm. I'm not sure that index_create having the new subid parameters is > > > good. And the last if(OidIsValid) clause handles storage persistence > > > so I did that there. But I don't strongly against it. > > Agreed. My choice there was not a clear improvement. > > > The change on alter_table.sql and create_table.sql is expecting to > > cause assertion failure. Don't we need that kind of explanation in > > the comment? > > Test comments generally describe the feature unique to that test, not how the > test might break. Some tests list bug numbers, but that doesn't apply here. Agreed. > > In swap_relation_files, we can remove rel2-related code when #ifndef > > USE_ASSERT_CHECKING. > > When state is visible to many compilation units, we should avoid making that > state depend on --enable-cassert. That would be a recipe for a Heisenbug. In > a hot code path, it might be worth the risk. I aggree that the new #ifdef can invite a Heisenbug. I thought that you didn't want that because it doesn't make substantial difference. If we decide to keep the consistency there, I would like to describe the code is there for consistency, not for the benefit of a specific assertion. (cluster.c:1116) -* new. The next step for rel2 is deletion, but copy rd_*Subid for the -* benefit of AssertPendingSyncs_RelationCache(). +* new. The next step for rel2 is deletion, but copy rd_*Subid for the +* consistency of the fieles. It is checked later by +* AssertPendingSyncs_RelationCache(). > > The patch adds the test for createSubid to pg_visibility.out. It > > doesn't fail without CLOBBER_CACHE_ALWAYS while regression test but > > CLOBBER_CACHE_ALWAYS causes initdb fail and the added check won't be > > reached. I'm not sure it is useful. > > I agree it's not clearly useful, but tests don't need to meet a "clearly > useful" standard. When a fast test is not clearly redundant with another > test, we generally accept it. In the earlier patch version that inspired this > test, RELCACHE_FORCE_RELEASE sufficed to make it fail. > > > config.sgml: > > +When wal_level is minimal > > and a > > +transaction commits after creating or rewriting a permanent table, > > +materialized view, or index, this setting determines how to persist > > > > "creating or truncation" a permanent table? and maybe "refreshing > > matview and reindex". I'm not sure that they can be merged that way. ... > I like mentioning truncation, but I dislike how this implies that CREATE > INDEX, CREATE MATERIALIZED VIEW, and ALTER INDEX SET TABLESPACE aren't in > scope. While I usually avoid the word "relation" in documentation, I can > justify it here to make the sentence less complex. How about the following? > > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -2484,9 +2484,9 @@ include_dir 'conf.d' > In minimal level, no information is logged for > -tables or indexes for the remainder of a transaction that creates or > -truncates them. This can make bulk operations much faster (see > -). But minimal WAL does not contain > -enough information to reconstruct the data from a base backup and the > -WAL logs, so replica or higher must be used to > -enable WAL archiving () and > -streaming replication. > +permanent relations for the remainder of a transaction that creates, > +rewrites, or truncates them. This can make bulk operations much > +faster (see ). But minimal WAL does > +not contain enough information to reconstruct the data from a base > +backup and the WAL logs, so replica or higher must > +be used to enable WAL archiving () > +and streaming replication. > > @@ -2891,9 +2891,9 @@ include_dir 'conf.d' > When wal_level is minimal and a > -transaction commits after creating or rewriting a permanent table, > -materialized view, or index, this setting determines how to persist > -the new data. If the data is smaller than this setting, write it to > -the WAL log; otherwise, use an fsync of the data file. Depending on > -the properties of your storage, raising or lowering this value might > -help if such commits are slowing concurrent transactions. The > default > -is two megabytes (2MB). >
Re: [Patch] Make pg_checksums skip foreign tablespace directories
Hi Michael,
On 2/24/20 7:26 AM, Michael Paquier wrote:
On Sun, Feb 23, 2020 at 04:08:58PM +0900, Michael Paquier wrote:
Good idea. Let's do things as you suggest.
Applied and back-patched this one down to 11.
FWIW, we took a slightly narrower approach to this issue in the
pgBackRest patch (attached).
I don't have an issue with the prefix approach since it works and the
Postgres project is very likely to catch it if there is a change in
behavior.
For third-party projects, though, it might pay to be more conservative
in case the behavior changes in the future, i.e.
pg_internal.init[something] (but not pg_internal\.init[0-9]+) becomes valid.
Regards,
--
-David
da...@pgmasters.net
diff --git a/doc/xml/release.xml b/doc/xml/release.xml
index a9a61bc0..0089d222 100644
--- a/doc/xml/release.xml
+++ b/doc/xml/release.xml
@@ -67,6 +67,16 @@
+
+
+
+
+
+
+
+Skip pg_internal.init temp file during
backup.
+
+
@@ -8039,6 +8049,11 @@
mibiio
+
+Michael
Paquier
+michaelpq
+
+
Michael Renner
terrorobe
diff --git a/src/info/manifest.c b/src/info/manifest.c
index a5662fd6..1eae49af 100644
--- a/src/info/manifest.c
+++ b/src/info/manifest.c
@@ -579,8 +579,13 @@ manifestBuildCallback(void *data, const StorageInfo *info)
strPtr(manifestName));
}
-// Skip pg_internal.init since it is recreated on startup
-if (strEqZ(info->name, PG_FILE_PGINTERNALINIT))
+// Skip pg_internal.init since it is recreated on startup. It's
also possible, (though unlikely) that a temp file with
+// the creating process id as the extension can exist so skip that
as well. This seems to be a bug in PostgreSQL since
+// the temp file should be removed on startup. Use
regExpMatchOne() here instead of preparing a regexp in advance since
+// the likelihood of needing the regexp should be very small.
+if ((pgVersion <= PG_VERSION_84 || buildData.dbPath) &&
strBeginsWithZ(info->name, PG_FILE_PGINTERNALINIT) &&
+(strSize(info->name) == sizeof(PG_FILE_PGINTERNALINIT) - 1 ||
+regExpMatchOne(STRDEF("\\.[0-9]+"), strSub(info->name,
sizeof(PG_FILE_PGINTERNALINIT) - 1
{
FUNCTION_TEST_RETURN_VOID();
return;
diff --git a/test/src/module/info/manifestTest.c
b/test/src/module/info/manifestTest.c
index 684905d1..9d945152 100644
--- a/test/src/module/info/manifestTest.c
+++ b/test/src/module/info/manifestTest.c
@@ -252,6 +252,10 @@ testRun(void)
// global directory
storagePathCreateP(storagePgWrite, STRDEF(PG_PATH_GLOBAL), .mode =
0700, .noParentCreate = true);
storagePutP(storageNewWriteP(storagePgWrite, STRDEF(PG_PATH_GLOBAL "/"
PG_FILE_PGINTERNALINIT)), NULL);
+storagePutP(storageNewWriteP(storagePgWrite, STRDEF(PG_PATH_GLOBAL "/"
PG_FILE_PGINTERNALINIT ".1")), NULL);
+storagePutP(
+storageNewWriteP(storagePgWrite, STRDEF(PG_PATH_GLOBAL "/"
PG_FILE_PGINTERNALINIT ".allow"), .modeFile = 0400,
+.timeModified = 1565282114), NULL);
storagePutP(
storageNewWriteP(storagePgWrite, STRDEF(PG_PATH_GLOBAL "/t1_1"),
.modeFile = 0400, .timeModified = 1565282114), NULL);
@@ -282,6 +286,7 @@ testRun(void)
"\n"
"[target:file]\n"
"pg_data/PG_VERSION={\"size\":4,\"timestamp\":1565282100}\n"
+
"pg_data/global/pg_internal.init.allow={\"master\":false,\"size\":0,\"timestamp\":1565282114}\n"
"pg_data/global/t1_1={\"master\":false,\"size\":0,\"timestamp\":1565282114}\n"
"pg_data/pg_dynshmem/BOGUS={\"size\":0,\"timestamp\":1565282101}\n"
"pg_data/pg_notify/BOGUS={\"size\":0,\"timestamp\":1565282102}\n"
@@ -419,6 +424,7 @@ testRun(void)
"pg_data/base/1/t1_1.1={\"size\":0,\"timestamp\":1565282113}\n"
"pg_data/base/1/t888_888_vm={\"size\":0,\"timestamp\":1565282113}\n"
"pg_data/base/1/t888_888_vm.99={\"size\":0,\"timestamp\":1565282113}\n"
+
"pg_data/global/pg_internal.init.allow={\"size\":0,\"timestamp\":1565282114}\n"
"pg_data/global/t1_1={\"size\":0,\"timestamp\":1565282114}\n"
"pg_data/pg_dynshmem/BOGUS={\"master\":true,\"size\":0,\"timestamp\":1565282101}\n"
"pg_data/pg_hba.conf={\"master\":true,\"size\":9,\"timestamp\":1565282117}\n"
@@ -537,6 +543,7 @@ testRun(void)
Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
Hi, On 2020-02-23 16:27:57 +0900, Michael Paquier wrote: > On Sat, Feb 22, 2020 at 10:22:27PM +0100, Daniel Gustafsson wrote: > > Turns out that we in heap_multi_insert missed to call log_heap_new_cid for > > the > > first tuple inserted, we only do it in the loop body for the subsequent > > ones. > > With the attached patch, the v6 of this patch posted upthead pass the tests > > for > > me. I have a v7 brewing which I'll submit shortly, but since this fix > > unrelated to that patchseries other than as a pre-requisite I figured I'd > > post > > that separately. Thanks for finding! > Good catch. I would not backpatch that as it is not a live bug > because heap_multi_insert() is not used for catalogs yet. With your > patch, that would be the case though.. Thanks for pushing. I do find it a bit odd that you essentially duplicated the existing comment in a different phrasing. What's the point? Greetings, Andres Freund
Re: Memory-Bounded Hash Aggregation
On 2020-02-22 11:02:16 -0800, Jeff Davis wrote: > On Sat, 2020-02-22 at 10:00 -0800, Andres Freund wrote: > > Both patches, or just 0013? Seems the earlier one might make the > > addition of the opcodes you add less verbose? > > Just 0013, thank you. 0008 looks like it will simplify things. Pushed 0008.
Re: Error on failed COMMIT
On Mon, 24 Feb 2020 at 17:59, Merlin Moncure wrote: > On Mon, Feb 24, 2020 at 4:06 PM Vladimir Sitnikov > wrote: > > > > Merlin>My biggest sense of alarm with the proposed change is that it > could > > Merlin>leave applications in a state where the transaction is hanging > there > > > > How come? > > The spec says commit ends the transaction. > > Can you please clarify where the proposed change leaves a hanging > transaction? > > > > Just in case, the proposed change is as follows: > > > > postgres=# begin; > > BEGIN > > postgres=# aslkdfasdf; > > ERROR: syntax error at or near "aslkdfasdf" > > LINE 1: aslkdfasdf; > > ^ > > postgres=# commit; > > ROLLBACK <-- this should be replaced with "ERROR: can't commit the > transaction because ..." > > postgres=# commit; > > WARNING: there is no transaction in progress <-- this should be as it > is currently. Even if commit throws an error, the transaction should be > terminated. > > COMMIT > > Ok, you're right; I missed the point in that it's not nearly as bad as > I thought you were suggesting (to treat commit as bad statement) but > the transaction would still terminate. Still, this is very sensitive > stuff, do you think most common connection poolers would continue to > work after making this change? > Don't see why not. All that happens is that an error message is emitted by the server on commit instead of silently rolling back Dave Cramer https://www.postgres.rocks
Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
> On 23 Feb 2020, at 08:27, Michael Paquier wrote: > > On Sat, Feb 22, 2020 at 10:22:27PM +0100, Daniel Gustafsson wrote: >> Turns out that we in heap_multi_insert missed to call log_heap_new_cid for >> the >> first tuple inserted, we only do it in the loop body for the subsequent ones. >> With the attached patch, the v6 of this patch posted upthead pass the tests >> for >> me. I have a v7 brewing which I'll submit shortly, but since this fix >> unrelated to that patchseries other than as a pre-requisite I figured I'd >> post >> that separately. > > Good catch. I would not backpatch that as it is not a live bug > because heap_multi_insert() is not used for catalogs yet. With your > patch, that would be the case though.. I'll leave that call up to others, the bug is indeed unreachable with the current coding. Attached is a v7 of the catalog multi_insert patch which removes some code duplication which was previously commented on. There are still a few rouch edges but this version passes tests when paired with the heap_multi_insert cid patch. cheers ./daniel catalog_multi_insert-v7.patch Description: Binary data heap_multi_insert_cid.patch Description: Binary data
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 4:06 PM Vladimir Sitnikov wrote: > > Merlin>My biggest sense of alarm with the proposed change is that it could > Merlin>leave applications in a state where the transaction is hanging there > > How come? > The spec says commit ends the transaction. > Can you please clarify where the proposed change leaves a hanging transaction? > > Just in case, the proposed change is as follows: > > postgres=# begin; > BEGIN > postgres=# aslkdfasdf; > ERROR: syntax error at or near "aslkdfasdf" > LINE 1: aslkdfasdf; > ^ > postgres=# commit; > ROLLBACK <-- this should be replaced with "ERROR: can't commit the > transaction because ..." > postgres=# commit; > WARNING: there is no transaction in progress <-- this should be as it is > currently. Even if commit throws an error, the transaction should be > terminated. > COMMIT Ok, you're right; I missed the point in that it's not nearly as bad as I thought you were suggesting (to treat commit as bad statement) but the transaction would still terminate. Still, this is very sensitive stuff, do you think most common connection poolers would continue to work after making this change? merlin
Re: Improve search for missing parent downlinks in amcheck
Hi!
Thank you for your review. The revised version is attached.
On Wed, Feb 19, 2020 at 1:16 AM Peter Geoghegan wrote:
> On Tue, Feb 18, 2020 at 2:16 AM Alexander Korotkov
> wrote:
> > > Don't need the "!P_ISLEAF()" here.
> >
> > Why don't I need. bt_downlink_connectivity_check() checks one level
> > down to the target level. But there is no one level down to leaf...
>
> Because offset_is_negative_infinity() checks P_ISLEAF() for you. Maybe
> it's better your way, though -- apparently it's clearer.
Oh, I see. I prefer to leave it my way. Explicit check is nearly
free and makes it clearer for me.
> > > > Alternatively
> > > > +* we might find child with high key while traversing from
> > > > +* previous downlink to current one. Then matching key
> > > > resides
> > > > +* the same offset number as current downlink.
> > > > +*/
> > >
> > > Not sure what "traversing from previous downlink to current one" means at
> > > all.
> >
> > I've rephrased this comment, please check.
>
> > Agree, replaced _bt_compare() with bt_pivot_tuple_identical(). It
> > becomes even simpler now, thanks!
>
> There was actually an even better reason to invent
> bt_pivot_tuple_identical(): a call to _bt_compare() in amcheck needs
> to do something like the extra steps that you see in routines like
> invariant_l_offset(). _bt_compare() will return 0 when the insertion
> scankey has a prefix of scankey/column values that are equal, even
> though there may be additional columns in the index tuple that are not
> compared. So, you could have a truncated multi-column high key that is
> "equal" to pivot tuple in parent that is actually to the right in the
> key space. This blind spot would often occur with low cardinality
> indexes, where we often have something like this in pivot tuples on
> internal pages:
>
> 'foo, -inf'
> 'foo, (1,24)'
> 'food, -inf'. <-- This pivot tuple's downlink points to the final leaf
> page that's filled with duplicates of the value 'foo'
> 'food, (3,19)' <-- This pivot tuple's downlink points to the *first*
> leaf page that's filled with duplicates of the value 'food'
> ...
>
> The situation is really common in low cardinality indexes because
> nbtsplitloc.c hates splitting a leaf page between two duplicates -- it
> is avoided whenever possible. You reliably get a '-inf' value for the
> TID in the first pivot tuple for the duplicate, followed by a real
> heap TID for later pivot tuples for pages with the same duplicate
> value.
>
Thank you for the explanation!
> > > I think bt_downlink_check() and bt_downlink_connectivity_check()
> > > should be renamed to something broader. In my mind, downlink is
> > > basically a block number. We have been sloppy about using the term
> > > downlink when we really mean "pivot tuple with a downlink" -- I am
> > > guilty of this myself. But it seems more important, now that you have
> > > the new high key check.
> >
> > Hmm... Names are hard for me. I didn't do any renaming for now. What
> > about this?
> > bt_downlink_check() => bt_child_check()
> > bt_downlink_connectivity_check() => bt_child_connectivity_highkey_check()
>
> I suggest:
>
> bt_downlink_check() => bt_child_check()
> bt_downlink_connectivity_check() => bt_child_highkey_check()
>
> While bt_downlink_connectivity_check() moves right on the target's
> child level, this isn't the common case. Moving right like that
> doesn't need to be suggested by the name of the function.
>
> Most of the time, we just check the high key -- right?
Good. Renamed as you proposed.
> * You should say "target page lsn" here instead:
>
> pg@tpce:5432 [19852]=# select bt_index_parent_check(:'idxrelation',true,
> true);
> ERROR: mismatch between parent key and child high key in index "i_holding2"
> DETAIL: Target block=1570 child block=1690 parent page lsn=0/0.
> Time: 12.509 ms
Yep, fixed.
> * Maybe say "Move to the right on the child level" in a comment above
> the bt_downlink_connectivity_check() "while (true)" loop here:
Comment is added.
> > +
> > + while (true)
> > + {
> > + /*
> > +* Did we traverse the whole tree level and this is check for pages
> > to
> > +* the right of rightmost downlink?
> > +*/
>
> * If you are going to save a low key for the target page in memory,
> then you only need to do so for "state->readonly"/parent verification.
Sure, now it's just for state->readonly.
> * You should s/lokey/lowkey/ -- I prefer the spelling "lowkey" or "low
> key". This is a term that nbtsort.c now uses, in case you didn't know.
Changed, thank you for pointing.
> * The reason for saving a low key for each target page is very
> unclear. Can we fix this?
Actually, lowkey is used for removing "cousin page verification blind
spot" when there are incomplete splits. It might happen that we read
child with hikey matching its parent high key only when
bt_child_highkey_check() is called for "minus infinity" key of parent
right
Re: Error on failed COMMIT
On 12/02/2020 00:27, Tom Lane wrote: > Vik Fearing writes: >> On 11/02/2020 23:35, Tom Lane wrote: >>> So I assume you're imagining that that would leave us still in >>> transaction-aborted state, and the session is basically dead in >>> the water until the user thinks to issue ROLLBACK instead? > >> Actually, I was imagining that it would end the transaction as it does >> today, just with an error code. >> This is backed up by General Rule 9 which says "The current >> SQL-transaction is terminated." > > Hm ... that would be sensible, but I'm not entirely convinced. There > are several preceding rules that say that an exception condition is > raised, and normally you can stop reading at that point; nothing else > is going to happen. If COMMIT acts specially in this respect, they > ought to say so. Reading some more, I believe they do say so. SQL:2016-2 Section 4.41 SQL-transactions: If an SQL-transaction is terminated by a or unsuccessful execution of a , then all changes made to SQL-data or schemas by that SQL-transaction are canceled. This to me says that an unsuccessful COMMIT still terminates the transaction. -- Vik Fearing
Re: Error on failed COMMIT
Merlin>My biggest sense of alarm with the proposed change is that it could Merlin>leave applications in a state where the transaction is hanging there How come? The spec says commit ends the transaction. Can you please clarify where the proposed change leaves a hanging transaction? Just in case, the proposed change is as follows: postgres=# begin; BEGIN postgres=# aslkdfasdf; ERROR: syntax error at or near "aslkdfasdf" LINE 1: aslkdfasdf; ^ postgres=# commit; ROLLBACK <-- this should be replaced with "ERROR: can't commit the transaction because ..." postgres=# commit; WARNING: there is no transaction in progress <-- this should be as it is currently. Even if commit throws an error, the transaction should be terminated. COMMIT No-one on the thread suggests the transaction must hang forever. Of course, commit must terminate the transaction one way or another. The proposed change is to surface the exception if user tries to commit or prepare a transaction that can't be committed. Note: the reason does not matter much. If deferred constraint fails on commit, then commit itself throws an error. Making commit throw an error in case "current transaction is aborted" makes perfect sense. Note: the same thing is with PREPARE TRANSACTION 'txname`. Apparently it silently responses with ROLLBACK which is strange as well. Vladimir
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Alvaro Herrera writes: > On 2020-Feb-24, Tom Lane wrote: >> Why not just drive it off max_files_per_process? On Unix, that >> largely exists to override the ulimit setting anyway. With no >> comparable knob on a Windows system, we might as well just say >> that's what you set. > That makes sense to me -- but if we do that, then maybe we should be > doing the setrlimit() dance on it too, on Linux^W^W where supported. Yeah, arguably we could try to setrlimit if max_files_per_process is larger than the ulimit. We should definitely not reduce the ulimit if max_files_per_process is smaller, though, since the DBA might intentionally be leaving daylight for purposes such as FD-hungry PL functions. On the whole I'm inclined to leave well enough alone on the Unix side --- there's nothing there that the DBA can't set if she wishes. regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
On 2020-Feb-24, Tom Lane wrote: > Alvaro Herrera writes: > > Then again, that would be akin to setrlimit() on Linux. Maybe we can > > consider that a separate GUC, in a separate patch, with a > > platform-specific default value that just corresponds to the OS's > > default, and the user can set to whatever suits them; then we call > > either _setmaxstdio() or setrlimit(). > > Why not just drive it off max_files_per_process? On Unix, that > largely exists to override the ulimit setting anyway. With no > comparable knob on a Windows system, we might as well just say > that's what you set. That makes sense to me -- but if we do that, then maybe we should be doing the setrlimit() dance on it too, on Linux^W^W where supported. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Alvaro Herrera writes: > On 2020-Feb-24, Tom Lane wrote: >> I don't think there's much point in telling Windows users about >> _setmaxstdio() here. > Yeah, telling users to _setmaxstdio() themselves is useless, because > they can't do it; that's something *we* should do. I think the 512 > limit is a bit low; why not increase that a little bit? Maybe just to > the Linux default of 1024. > Then again, that would be akin to setrlimit() on Linux. Maybe we can > consider that a separate GUC, in a separate patch, with a > platform-specific default value that just corresponds to the OS's > default, and the user can set to whatever suits them; then we call > either _setmaxstdio() or setrlimit(). Why not just drive it off max_files_per_process? On Unix, that largely exists to override the ulimit setting anyway. With no comparable knob on a Windows system, we might as well just say that's what you set. regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
On 2020-Feb-24, Tom Lane wrote:
> I wrote:
> > I thought about platform-specific messages, but it's not clear to me
> > whether our translation infrastructure will find messages that are
> > inside #ifdefs ... anyone know?
>
> Oh, but of course it does. So let's do
>
> errdetail("There are too many open files on the local server."),
> #ifndef WIN32
> errhint("Raise the server's max_files_per_process and/or \"ulimit
> -n\" limits.")
> #else
> errhint("Raise the server's max_files_per_process setting.")
> #endif
WFM.
> I don't think there's much point in telling Windows users about
> _setmaxstdio() here.
Yeah, telling users to _setmaxstdio() themselves is useless, because
they can't do it; that's something *we* should do. I think the 512
limit is a bit low; why not increase that a little bit? Maybe just to
the Linux default of 1024.
Then again, that would be akin to setrlimit() on Linux. Maybe we can
consider that a separate GUC, in a separate patch, with a
platform-specific default value that just corresponds to the OS's
default, and the user can set to whatever suits them; then we call
either _setmaxstdio() or setrlimit().
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
I wrote:
> Alvaro Herrera writes:
>> As for the platform dependencies, I see two main options: make the hint
>> platform-specific (i.e have #ifdef branches per platform) or make it
>> even more generic, such as "file descriptor limits".
> I thought about platform-specific messages, but it's not clear to me
> whether our translation infrastructure will find messages that are
> inside #ifdefs ... anyone know?
Oh, but of course it does. So let's do
errdetail("There are too many open files on the local server."),
#ifndef WIN32
errhint("Raise the server's max_files_per_process and/or \"ulimit -n\"
limits.")
#else
errhint("Raise the server's max_files_per_process setting.")
#endif
I don't think there's much point in telling Windows users about
_setmaxstdio() here.
regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Andres Freund writes: > On 2020-02-24 16:14:53 -0300, Alvaro Herrera wrote: >> I suppose we do use the C runtime. There's a reference to >> _setmaxstdio() being able to raise the limit from the default of 512 to >> up to 8192 open files. We don't currently invoke that. >> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setmaxstdio?view=vs-2019 > If we ever go for that, we should also consider raising the limit on > unix systems up to the hard limit when hitting the fd ceiling. I.e. get > the current limit with getrlimit(RLIMIT_NOFILE) and raise rlim_cur > [closer] to rlim_max with setrlimit. I'm disinclined to think we should override the user's wishes in this way. Especially given PG's proven ability to run the kernel completely out of file descriptors ... regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Hi, On 2020-02-24 16:14:53 -0300, Alvaro Herrera wrote: > But the C runtime does: > https://docs.microsoft.com/en-us/cpp/c-runtime-library/file-handling?view=vs-2019 > I suppose we do use the C runtime. There's a reference to > _setmaxstdio() being able to raise the limit from the default of 512 to > up to 8192 open files. We don't currently invoke that. > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setmaxstdio?view=vs-2019 If we ever go for that, we should also consider raising the limit on unix systems up to the hard limit when hitting the fd ceiling. I.e. get the current limit with getrlimit(RLIMIT_NOFILE) and raise rlim_cur [closer] to rlim_max with setrlimit. Perhaps it'd even be worthwhile to just always raise the limit, if possible, in set_max_safe_fds(), by max_safe_fds + NUM_RESERVED_FDS. That way PLs, other shared libs, would have a more usualy amount of FDs available. Rather than a fairly small number, but only when the backend has been running for a while in the right workload. Greetings, Andres Freund
Re: Error on failed COMMIT
On Sun, Feb 23, 2020 at 7:59 PM Dave Cramer wrote: > > I think the fact that this is a violation of the SQL SPEC lends considerable > credence to the argument for changing the behaviour. > Since this can lead to losing a transaction I think there is even more reason > to look at changing the behaviour. The assumption that COMMIT terminates the transaction is going to be deeply embedded into many applications. It's just too convenient not to rely on. For example, I maintain a bash based deployment framework that assembles large SQL files from bit and pieces and tacks a COMMIT at the end. It's not *that* much work to test for failure and add a rollback but it's the kind of surprise our users hate during the upgrade process. Over the years we've tightened the behavior of postgres to be inline with the spec (example: Tom cleaned up the row-wise comparison behavior in 8.2) but in other cases we had to punt (IS NULL/coalesce disagreement over composites for example), identifier case sensitivity etc. The point is, changing this stuff can be really painful and we have to evaluate the benefits vs the risks. My biggest sense of alarm with the proposed change is that it could leave applications in a state where the transaction is hanging there it could previously assume it had resolved; this could be catastrophic in impact in certain real world scenarios. Tom is right, a GUC is the equivalent of "sweeping the problem under the wrong" (if you want examples of the long term consequences of that vision read through this: https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html). The value proposition of the change is however a little light relative to the risks IMO. I do think we need to have good page summarizing non-spec behaviors in the documentation however. merlin
Re: plan cache overhead on plpgsql expression
po 24. 2. 2020 v 18:56 odesílatel Pavel Stehule napsal: > > > po 24. 2. 2020 v 18:47 odesílatel Pavel Stehule > napsal: > >> >> >> čt 20. 2. 2020 v 20:15 odesílatel Pavel Stehule >> napsal: >> >>> >>> >>> st 19. 2. 2020 v 8:09 odesílatel Amit Langote >>> napsal: >>> On Wed, Feb 19, 2020 at 3:56 PM Amit Langote wrote: > On Wed, Feb 19, 2020 at 3:38 PM Pavel Stehule < pavel.steh...@gmail.com> wrote: > > st 19. 2. 2020 v 7:30 odesílatel Pavel Stehule < pavel.steh...@gmail.com> napsal: > >> út 18. 2. 2020 v 17:08 odesílatel Amit Langote < amitlangot...@gmail.com> napsal: > >>> > I updated the patch to do that. > >>> > > >>> > With the new patch, `select foo()`, with inline-able sql_incr() in it, > >>> > runs in 679 ms. > >>> > > >>> > Without any inline-able function, it runs in 330 ms, whereas with > >>> > HEAD, it takes 590 ms. > >>> > >>> I polished it a bit. > >> > >> > >> the performance looks very interesting - on my comp the execution time of 1 iterations was decreased from 34 sec to 15 sec, > >> > >> So it is interesting speedup > > > > but regress tests fails > > Oops, I failed to check src/pl/plpgsql tests. > > Fixed in the attached. Added a regression test based on examples discussed here too. >>> >>> It is working without problems >>> >>> I think this patch is very interesting for Postgres 13 >>> >> >> I checked a performance of this patch again and I think so there is not >> too much space for another optimization - maybe JIT can help. >> >> There is relative high overhead of call of strict functions - the params >> are repeatedly tested against NULL. >> > > But I found one issue - I don't know if this issue is related to your > patch or plpgsql_check. > > plpgsql_check try to clean after it was executed - it cleans all plans. > But some pointers on simple expressions are broken after catched exceptions. > > expr->plan = 0x80. Is interesting, so other fields of this expressions are > correct. > I am not sure, but after patching the SPI_prepare_params the current memory context is some short memory context. Can SPI_prepare_params change current memory context? It did before. But after patching different memory context is active. Regards Pavel > > > > >> Regards >> >> Pavel >> >> >> >>> Regards >>> >>> Pavel >>> Thanks, Amit >>>
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Alvaro Herrera writes: > On 2020-Feb-24, Tom Lane wrote: >> We could also consider a HINT, along the lines of "Raise the server's >> max_files_per_process and/or \"ulimit -n\" limits." This again has >> the ambiguity about which server, and it also seems dangerously >> platform-specific. > Maybe talk about "the local server" to distinguish from the other one. OK by me. > As for the platform dependencies, I see two main options: make the hint > platform-specific (i.e have #ifdef branches per platform) or make it > even more generic, such as "file descriptor limits". I thought about platform-specific messages, but it's not clear to me whether our translation infrastructure will find messages that are inside #ifdefs ... anyone know? If that does work, I'd be inclined to mention ulimit -n on non-Windows and just say nothing about that on Windows. "File descriptor limits" seems too unhelpful here. regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
On 2020-Feb-24, Tom Lane wrote: > We could also consider a HINT, along the lines of "Raise the server's > max_files_per_process and/or \"ulimit -n\" limits." This again has > the ambiguity about which server, and it also seems dangerously > platform-specific. Maybe talk about "the local server" to distinguish from the other one. As for the platform dependencies, I see two main options: make the hint platform-specific (i.e have #ifdef branches per platform) or make it even more generic, such as "file descriptor limits". A quick search suggests that current Windows itself doesn't typically have such problems: https://stackoverflow.com/questions/31108693/increasing-no-of-file-handles-in-windows-7-64-bit https://docs.microsoft.com/es-es/archive/blogs/markrussinovich/pushing-the-limits-of-windows-handles But the C runtime does: https://docs.microsoft.com/en-us/cpp/c-runtime-library/file-handling?view=vs-2019 I suppose we do use the C runtime. There's a reference to _setmaxstdio() being able to raise the limit from the default of 512 to up to 8192 open files. We don't currently invoke that. https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setmaxstdio?view=vs-2019 -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: pg_trigger.tgparentid
On 2020-Feb-19, Amit Langote wrote:
> Or:
>
> * However, if our parent is a partition itself, there might be
> * internal triggers that must not be skipped. For example, triggers
> * on the parent that were in turn cloned from its own parent are
> * marked internal, which must be cloned to the partition.
Thanks for pointing this out -- I agree it needed rewording. I slightly
adjusted your text like this:
* Internal triggers require careful examination. Ideally, we
don't
* clone them. However, if our parent is itself a partition,
there
* might be internal triggers that must not be skipped; for
example,
* triggers on our parent that are in turn clones from its
parent (our
* grandparent) are marked internal, yet they are to be cloned.
Is this okay for you?
Tom Lane wrote:
> It'd be nice if the term "parent trigger" were defined somewhere in
> this. Seems all right otherwise.
Fair point. I propose to patch catalog.sgml like this
Parent trigger that this trigger is cloned from, zero if not a clone;
this happens when partitions are created or attached to a partitioned
table.
It's perhaps not great to have to explain the parentage concept in the
catalog docs, so I'm going to go over the other documentation pages
(trigger.sgml and ref/create_trigger.sgml) to see whether they need any
patching; it's possible that we neglected to update them properly when
the partitioning-related commits went it.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From ff13d7004c9ee77ba411ace405224188aa0c353c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 17 Feb 2020 17:34:47 -0300
Subject: [PATCH v2] Record parents of triggers
This let us get rid of a recently introduced ugly hack (commit
1fa846f1c9af).
---
doc/src/sgml/catalogs.sgml | 11 ++
src/backend/commands/tablecmds.c | 59 +++-
src/backend/commands/trigger.c | 1 +
src/include/catalog/pg_trigger.h | 1 +
4 files changed, 18 insertions(+), 54 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index a10b66569b..34bc0d0526 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6951,6 +6951,17 @@ SCRAM-SHA-256$iteration count:
The table this trigger is on
+
+ tgparentid
+ oid
+ pg_trigger.oid
+
+ Parent trigger that this trigger is cloned from, zero if not a clone;
+ this happens when partitions are created or attached to a partitioned
+ table.
+
+
+
tgname
name
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d663fc..02a7c04fdb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -16447,54 +16447,6 @@ out:
MemoryContextDelete(cxt);
}
-/*
- * isPartitionTrigger
- * Subroutine for CloneRowTriggersToPartition: determine whether
- * the given trigger has been cloned from another one.
- *
- * We use pg_depend as a proxy for this, since we don't have any direct
- * evidence. This is an ugly hack to cope with a catalog deficiency.
- * Keep away from children. Do not stare with naked eyes. Do not propagate.
- */
-static bool
-isPartitionTrigger(Oid trigger_oid)
-{
- Relation pg_depend;
- ScanKeyData key[2];
- SysScanDesc scan;
- HeapTuple tup;
- bool found = false;
-
- pg_depend = table_open(DependRelationId, AccessShareLock);
-
- ScanKeyInit([0], Anum_pg_depend_classid,
-BTEqualStrategyNumber,
-F_OIDEQ,
-ObjectIdGetDatum(TriggerRelationId));
- ScanKeyInit([1], Anum_pg_depend_objid,
-BTEqualStrategyNumber,
-F_OIDEQ,
-ObjectIdGetDatum(trigger_oid));
-
- scan = systable_beginscan(pg_depend, DependDependerIndexId,
- true, NULL, 2, key);
- while ((tup = systable_getnext(scan)) != NULL)
- {
- Form_pg_depend dep = (Form_pg_depend) GETSTRUCT(tup);
-
- if (dep->refclassid == TriggerRelationId)
- {
- found = true;
- break;
- }
- }
-
- systable_endscan(scan);
- table_close(pg_depend, AccessShareLock);
-
- return found;
-}
-
/*
* CloneRowTriggersToPartition
* subroutine for ATExecAttachPartition/DefineRelation to create row
@@ -16537,11 +16489,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition)
/*
* Internal triggers require careful examination. Ideally, we don't
- * clone them.
- *
- * However, if our parent is a partitioned relation, there might be
- * internal triggers that need cloning. In that case, we must skip
- * clone it if the trigger on parent depends on another trigger.
+ * clone them. However, if our parent is itself a partition, there
+ * might be internal triggers that must not be skipped; for example,
+ * triggers on our parent that are in turn
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Mark Dilger writes: >> On Feb 20, 2020, at 8:30 PM, Tom Lane wrote: >> This idea doesn't fix every possible problem. For instance, if you >> have a plperlu function that wants to open a bunch of files, I don't >> see any easy way to ensure we release VFDs to make that possible. >> But it's sure an improvement on the status quo. > I understand that you were using plperlu just as an example, but it got > me thinking. Could we ship a wrapper using perl's tie() mechanism to > call a new spi function to report when a file handle is opened and when > it is closed? Most plperlu functions would not participate, since > developers will not necessarily know to use the wrapper, but at least > they could learn about the wrapper and use it as a work-around if this > becomes a problem for them. Perhaps the same spi function could be used > by other procedural languages. Hmm. I had thought briefly about getting plperl to do that automatically and had concluded that I didn't see a way (though there might be one; I'm not much of a Perl expert). But if we accept that changes in the plperl function's source code might be needed, it gets a lot easier, for sure. Anyway, the point of the current patch is to provide the mechanism and use it in a couple of places where we know there's an issue. Improving the PLs is something that could be added later. > I can't see this solution working unless the backend can cleanup > properly under exceptional conditions, and decrement the counter of used > file handles appropriately. But that's the same requirement that > postgres_fdw would also have, right? Would the same mechanism work for > both? The hard part is to tie into whatever is responsible for closing the kernel FD. If you can ensure that the FD gets closed, you can put the ReleaseExternalFD() call at the same place(s). > Is this too convoluted to be worth the bother? So far we've not seen field reports of PL functions running out of FDs; and there's always the ad-hoc solution of making sure the server's ulimit -n limit is sufficiently larger than max_files_per_process. So I wouldn't put a lot of effort into it right now. But it's nice to have an idea about what to do if it does become a hot issue for somebody. regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Hi, On 2020-02-24 10:29:51 -0800, Mark Dilger wrote: > > On Feb 20, 2020, at 8:30 PM, Tom Lane wrote: > > > > This idea doesn't fix every possible problem. For instance, if you > > have a plperlu function that wants to open a bunch of files, I don't > > see any easy way to ensure we release VFDs to make that possible. > > But it's sure an improvement on the status quo. > > I understand that you were using plperlu just as an example, but it > got me thinking. Could we ship a wrapper using perl's tie() mechanism > to call a new spi function to report when a file handle is opened and > when it is closed? Most plperlu functions would not participate, > since developers will not necessarily know to use the wrapper, but at > least they could learn about the wrapper and use it as a work-around > if this becomes a problem for them. Perhaps the same spi function > could be used by other procedural languages. While we're thinking a bit outside of the box: We could just dup() a bunch of fds within fd.c to protect fd.c's fd "quota". And then close them when actually needing the fds. Not really suggesting that we go for that, but it does have some appeal. > I can't see this solution working unless the backend can cleanup properly > under exceptional conditions, and decrement the counter of used file handles > appropriately. But that's the same requirement that postgres_fdw would also > have, right? Would the same mechanism work for both? We can just throw an error, and all fdw state should get cleaned up. We can't generally rely on that for plperl, as it IIRC can global state. So I don't think they're in the same boat. Greetings, Andres Freund
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
> On Feb 20, 2020, at 8:30 PM, Tom Lane wrote: > > This idea doesn't fix every possible problem. For instance, if you > have a plperlu function that wants to open a bunch of files, I don't > see any easy way to ensure we release VFDs to make that possible. > But it's sure an improvement on the status quo. I understand that you were using plperlu just as an example, but it got me thinking. Could we ship a wrapper using perl's tie() mechanism to call a new spi function to report when a file handle is opened and when it is closed? Most plperlu functions would not participate, since developers will not necessarily know to use the wrapper, but at least they could learn about the wrapper and use it as a work-around if this becomes a problem for them. Perhaps the same spi function could be used by other procedural languages. I can't see this solution working unless the backend can cleanup properly under exceptional conditions, and decrement the counter of used file handles appropriately. But that's the same requirement that postgres_fdw would also have, right? Would the same mechanism work for both? I imagine something like ::IO::File and ::File::Temp which could be thin wrappers around IO::File and File::Temp that automatically do the tie()ing for you. (Replace with whichever name seems best.) Is this too convoluted to be worth the bother? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: plan cache overhead on plpgsql expression
po 24. 2. 2020 v 18:47 odesílatel Pavel Stehule napsal: > > > čt 20. 2. 2020 v 20:15 odesílatel Pavel Stehule > napsal: > >> >> >> st 19. 2. 2020 v 8:09 odesílatel Amit Langote >> napsal: >> >>> On Wed, Feb 19, 2020 at 3:56 PM Amit Langote >>> wrote: >>> > On Wed, Feb 19, 2020 at 3:38 PM Pavel Stehule >>> wrote: >>> > > st 19. 2. 2020 v 7:30 odesílatel Pavel Stehule < >>> pavel.steh...@gmail.com> napsal: >>> > >> út 18. 2. 2020 v 17:08 odesílatel Amit Langote < >>> amitlangot...@gmail.com> napsal: >>> > >>> > I updated the patch to do that. >>> > >>> > >>> > >>> > With the new patch, `select foo()`, with inline-able sql_incr() >>> in it, >>> > >>> > runs in 679 ms. >>> > >>> > >>> > >>> > Without any inline-able function, it runs in 330 ms, whereas with >>> > >>> > HEAD, it takes 590 ms. >>> > >>> >>> > >>> I polished it a bit. >>> > >> >>> > >> >>> > >> the performance looks very interesting - on my comp the execution >>> time of 1 iterations was decreased from 34 sec to 15 sec, >>> > >> >>> > >> So it is interesting speedup >>> > > >>> > > but regress tests fails >>> > >>> > Oops, I failed to check src/pl/plpgsql tests. >>> > >>> > Fixed in the attached. >>> >>> Added a regression test based on examples discussed here too. >>> >> >> It is working without problems >> >> I think this patch is very interesting for Postgres 13 >> > > I checked a performance of this patch again and I think so there is not > too much space for another optimization - maybe JIT can help. > > There is relative high overhead of call of strict functions - the params > are repeatedly tested against NULL. > But I found one issue - I don't know if this issue is related to your patch or plpgsql_check. plpgsql_check try to clean after it was executed - it cleans all plans. But some pointers on simple expressions are broken after catched exceptions. expr->plan = 0x80. Is interesting, so other fields of this expressions are correct. > Regards > > Pavel > > > >> Regards >> >> Pavel >> >>> >>> Thanks, >>> Amit >>> >>
Re: BUG #16108: Colorization to the output of command-line has unproperly behaviors at Windows platform
On Sat, Feb 22, 2020 at 9:09 PM Michail Nikolaev wrote: > > I am not sure it is good idea to mix both patches because it adds some > confusion and makes it harder to merge each. > Maybe is it better to update current patch the way to reuse some > function later in [1]? > The patch was originaly reported for Windows, but looking into Peter's patch, I think this issue affects other systems unless we use stricter logic to detect a colorable terminal when using the "auto" option. Probably, the way to go is leaving this patch as WIN32 only and thinking about a future patch. > Also, regarding comment > > It is disabled by default, so it must be enabled to use color outpout. > > It is not true for new terminal, for example. Maybe it is better to > rephrase it to something like: "Check if TV100 support if enabled and > attempt to enable if not". > The logic I have seen on new terminals is that VT100 is supported but disabled. Would you find clearer? "Attempt to enable VT100 sequence processing. If it is not possible consider it as unsupported." Please find attached a patch addressing these comments. Regards, Juan José Santamaría Flecha v4-0001-command-line-colorization-on-windows.patch Description: Binary data
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 06:40:16PM +0100, Vik Fearing wrote: > On 24/02/2020 18:37, David Fetter wrote: > > > If we'd done this from a clean sheet of paper, it would have been the > > right decision. We're not there, and haven't been for decades. > > OTOH, it's never too late to do the right thing. Some right things take a lot of prep work in order to actually be right things. This is one of them. Defaulting to SERIALIZABLE isolation is another. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: Unicode escapes with any backend encoding
I wrote: > [ unicode-escapes-with-other-server-encodings-2.patch ] I see this patch got sideswiped by the recent refactoring of JSON lexing. Here's an attempt at fixing it up. Since the frontend code isn't going to have access to encoding conversion facilities, this creates a difference between frontend and backend handling of JSON Unicode escapes, which is mildly annoying but probably isn't going to bother anyone in the real world. Outside of jsonapi.c, there are no changes from v2. regards, tom lane diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml index 1b6aaf0..a9c68c7 100644 --- a/doc/src/sgml/json.sgml +++ b/doc/src/sgml/json.sgml @@ -61,8 +61,8 @@ - PostgreSQL allows only one character set - encoding per database. It is therefore not possible for the JSON + RFC 7159 specifies that JSON strings should be encoded in UTF8. + It is therefore not possible for the JSON types to conform rigidly to the JSON specification unless the database encoding is UTF8. Attempts to directly include characters that cannot be represented in the database encoding will fail; conversely, @@ -77,13 +77,13 @@ regardless of the database encoding, and are checked only for syntactic correctness (that is, that four hex digits follow \u). However, the input function for jsonb is stricter: it disallows - Unicode escapes for non-ASCII characters (those above U+007F) - unless the database encoding is UTF8. The jsonb type also + Unicode escapes for characters that cannot be represented in the database + encoding. The jsonb type also rejects \u (because that cannot be represented in PostgreSQL's text type), and it insists that any use of Unicode surrogate pairs to designate characters outside the Unicode Basic Multilingual Plane be correct. Valid Unicode escapes - are converted to the equivalent ASCII or UTF8 character for storage; + are converted to the equivalent single character for storage; this includes folding surrogate pairs into a single character. @@ -96,9 +96,8 @@ not jsonb. The fact that the json input function does not make these checks may be considered a historical artifact, although it does allow for simple storage (without processing) of JSON Unicode - escapes in a non-UTF8 database encoding. In general, it is best to - avoid mixing Unicode escapes in JSON with a non-UTF8 database encoding, - if possible. + escapes in a database encoding that does not support the represented + characters. @@ -144,8 +143,8 @@ string text -\u is disallowed, as are non-ASCII Unicode - escapes if database encoding is not UTF8 +\u is disallowed, as are Unicode escapes + representing characters not available in the database encoding number diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml index c908e0b..e134877 100644 --- a/doc/src/sgml/syntax.sgml +++ b/doc/src/sgml/syntax.sgml @@ -189,6 +189,23 @@ UPDATE "my_table" SET "a" = 5; ampersands. The length limitation still applies. + +Quoting an identifier also makes it case-sensitive, whereas +unquoted names are always folded to lower case. For example, the +identifiers FOO, foo, and +"foo" are considered the same by +PostgreSQL, but +"Foo" and "FOO" are +different from these three and each other. (The folding of +unquoted names to lower case in PostgreSQL is +incompatible with the SQL standard, which says that unquoted names +should be folded to upper case. Thus, foo +should be equivalent to "FOO" not +"foo" according to the standard. If you want +to write portable applications you are advised to always quote a +particular name or never quote it.) + + Unicode escape in identifiers @@ -230,7 +247,8 @@ U"d!0061t!+61" UESCAPE '!' The escape character can be any single character other than a hexadecimal digit, the plus sign, a single quote, a double quote, or a whitespace character. Note that the escape character is -written in single quotes, not double quotes. +written in single quotes, not double quotes, +after UESCAPE. @@ -239,32 +257,18 @@ U"d!0061t!+61" UESCAPE '!' -The Unicode escape syntax works only when the server encoding is -UTF8. When other server encodings are used, only code -points in the ASCII range (up to \007F) can be -specified. Both the 4-digit and the 6-digit form can be used to +Either the 4-digit or the 6-digit escape form can be used to specify UTF-16 surrogate pairs to compose characters with code points larger than U+, although the availability of the 6-digit form technically makes this unnecessary. (Surrogate -pairs are not stored directly, but combined into a single -code point that is then encoded in UTF-8.) +pairs are
Re: plan cache overhead on plpgsql expression
čt 20. 2. 2020 v 20:15 odesílatel Pavel Stehule napsal: > > > st 19. 2. 2020 v 8:09 odesílatel Amit Langote > napsal: > >> On Wed, Feb 19, 2020 at 3:56 PM Amit Langote >> wrote: >> > On Wed, Feb 19, 2020 at 3:38 PM Pavel Stehule >> wrote: >> > > st 19. 2. 2020 v 7:30 odesílatel Pavel Stehule < >> pavel.steh...@gmail.com> napsal: >> > >> út 18. 2. 2020 v 17:08 odesílatel Amit Langote < >> amitlangot...@gmail.com> napsal: >> > >>> > I updated the patch to do that. >> > >>> > >> > >>> > With the new patch, `select foo()`, with inline-able sql_incr() >> in it, >> > >>> > runs in 679 ms. >> > >>> > >> > >>> > Without any inline-able function, it runs in 330 ms, whereas with >> > >>> > HEAD, it takes 590 ms. >> > >>> >> > >>> I polished it a bit. >> > >> >> > >> >> > >> the performance looks very interesting - on my comp the execution >> time of 1 iterations was decreased from 34 sec to 15 sec, >> > >> >> > >> So it is interesting speedup >> > > >> > > but regress tests fails >> > >> > Oops, I failed to check src/pl/plpgsql tests. >> > >> > Fixed in the attached. >> >> Added a regression test based on examples discussed here too. >> > > It is working without problems > > I think this patch is very interesting for Postgres 13 > I checked a performance of this patch again and I think so there is not too much space for another optimization - maybe JIT can help. There is relative high overhead of call of strict functions - the params are repeatedly tested against NULL. Regards Pavel > Regards > > Pavel > >> >> Thanks, >> Amit >> >
Re: Error on failed COMMIT
On 24/02/2020 18:37, David Fetter wrote: > If we'd done this from a clean sheet of paper, it would have been the > right decision. We're not there, and haven't been for decades. OTOH, it's never too late to do the right thing. -- Vik Fearing
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 06:04:28PM +0530, Robert Haas wrote: > On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky wrote: > > As Dave wrote, the problem here isn't with the driver, but with framework > > or user-code which swallows the initial exception and allows code to > > continue to the commit. Npgsql (and I'm sure the JDBC driver too) does > > surface PostgreSQL errors as exceptions, and internally tracks the > > transaction status provided in the CommandComplete message. That means > > users have the ability - but not the obligation - to know about failed > > transactions, and some frameworks or user coding patterns could lead to a > > commit being done on a failed transaction. > > Agreed. All of that can be fixed in the driver, though. > > > If we think the current *user-visible* behavior is problematic (commit on > > failed transaction completes without throwing), then the only remaining > > question is where this behavior should be fixed - at the server or at the > > driver. As I wrote above, from the user's perspective it makes no > > difference - the change would be identical (and just as breaking) either > > way. So while drivers *could* implement the new behavior, what advantages > > would that have over doing it at the server? Some disadvantages do seem > > clear (repetition of the logic across each driver - leading to > > inconsistency across drivers, changing semantics at the driver by turning a > > non-error into an exception...). > > The advantage is that it doesn't cause a compatibility break. > > > > Well, it seems quite possible that there are drivers and applications > > > that don't have this issue; I've never had a problem with this behavior, > > > and I've been using PostgreSQL for something like two decades [...] > > > > If we are assuming that most user code is already written to avoid > > committing on failed transactions (by tracking transaction state etc.), > > then making this change at the server wouldn't affect those applications; > > the only applications affected would be those that do commit on failed > > transactions today, and it could be argued that those are likely to be > > broken today (since drivers today don't really expose the rollback in an > > accessible/discoverable way). > > libpq exposes it just fine, so I think you're overgeneralizing here. > > As I said upthread, I think one of the things that would be pretty > badly broken by this is psql -f something.sql, where something.sql > contains a series of blocks of the form "begin; something; something; > something; commit;". Right now whichever transactions succeed get > committed. With the proposed change, if one transaction block fails, > it'll merge with all of the following blocks. You may think that > nobody is doing this sort of thing, but I think people are, and that > they will come after us with pitchforks if we break it. I'm doing it, and I don't know about pitchforks, but I do know about suddenly needing to rewrite (and re-test, and re-integrate, and re-test some more) load-bearing code, and I'm not a fan of it. If we'd done this from a clean sheet of paper, it would have been the right decision. We're not there, and haven't been for decades. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: pgsql: Allow running src/tools/msvc/mkvcbuild.pl under not Windows
On 2020-Feb-21, Peter Eisentraut wrote: > Allow running src/tools/msvc/mkvcbuild.pl under not Windows > > This to allow verifying the MSVC build file generation without having > to have Windows. I suggest that src/tools/msvc/README should indicate how to use this; I don't think it's completely obvious. Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: client-side fsync() error handling
On 2020-02-21 05:18, Michael Paquier wrote: On Thu, Feb 20, 2020 at 10:10:11AM +0100, Peter Eisentraut wrote: OK, added in new patch. Thanks, that looks good. committed The frontends do neither right now, or at least the error handling is very inconsistent and inscrutable. It would be possible in theory to add a retry option, but that would be a very different patch, and given what we have learned about fsync(), it probably wouldn't be widely useful. Perhaps. Let's have this discussion later if there are drawbacks about changing things the way your patch does. If we don't do that, we'll never know about it either and this patch makes things safer. yup -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: PG v12.2 - Setting jit_above_cost is causing the server to crash
Aditya Toshniwal writes: > On Mon, Feb 24, 2020 at 12:46 PM Andres Freund wrote: >> This isn't reproducible here. Are you sure that you're running on a >> clean installation? > Yes I did a fresh installation using installer provided here - > https://www.enterprisedb.com/downloads/postgresql There is apparently something wrong with the JIT stuff in EDB's 12.2 build for macOS. At least, that's the conclusion I came to after off-list discussion with the submitter of bug #16264, which has pretty much exactly this symptom (especially if you're seeing "signal 9" reports in the postmaster log). For him, either disabling JIT or reverting to 12.1 made it go away. regards, tom lane
Re: pgsql: Add kqueue(2) support to the WaitEventSet API.
Thomas Munro writes:
> I suppose there may be users who have set ulimit -n high enough to
> support an FDW workload that connects to very many hosts, who will now
> need to set max_files_per_process higher to avoid the new error now
> that we're doing this accounting. That doesn't seem to be a problem
> in itself, but I wonder if the error message should make it clearer
> that it's our limit they hit here.
I struggled with the wording of that message, actually. The patch
proposes
+ereport(ERROR,
+
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not connect to server \"%s\"",
+server->servername),
+ errdetail("There are too many open files.")));
I wanted to say "The server has too many open files." but in context
it would seem to be talking about the remote server, so I'm not sure
how to fix that.
We could also consider a HINT, along the lines of "Raise the server's
max_files_per_process and/or \"ulimit -n\" limits." This again has
the ambiguity about which server, and it also seems dangerously
platform-specific.
regards, tom lane
Re: [Proposal] Global temporary tables
> 2020年2月24日 下午9:34，Prabhat Sahu 写道： > > Hi All, > > I observe a different behavior in "temporary table" and "global temporary > table". > Not sure if it is expected? > > postgres=# create global temporary table parent1(a int) on commit delete > rows; > CREATE TABLE > postgres=# create global temporary table child1() inherits (parent1); > CREATE TABLE > postgres=# insert into parent1 values(1); > INSERT 0 1 > postgres=# insert into child1 values(2); > INSERT 0 1 > postgres=# select * from parent1; > a > --- > (0 rows) > > postgres=# select * from child1; > a > --- > (0 rows) Because child1 inherits its father's on commit property. I can make GTT behave like local temp table. > > > postgres=# create temporary table parent2(a int) on commit delete rows; > CREATE TABLE > postgres=# create temporary table child2() inherits (parent2); > CREATE TABLE > postgres=# insert into parent2 values(1); > INSERT 0 1 > postgres=# insert into child2 values(2); > INSERT 0 1 > postgres=# select * from parent2; > a > --- > 2 > (1 row) > > postgres=# select * from child2; > a > --- > 2 > (1 row) > > > Thanks, > Prabhat Sahu >
Re: [Proposal] Global temporary tables
po 24. 2. 2020 v 14:34 odesílatel Prabhat Sahu < prabhat.s...@enterprisedb.com> napsal: > Hi All, > > I observe a different behavior in "temporary table" and "global temporary > table". > Not sure if it is expected? > > postgres=# create global temporary table parent1(a int) on commit delete > rows; > CREATE TABLE > postgres=# create global temporary table child1() inherits (parent1); > CREATE TABLE > postgres=# insert into parent1 values(1); > INSERT 0 1 > postgres=# insert into child1 values(2); > INSERT 0 1 > postgres=# select * from parent1; > a > --- > (0 rows) > > postgres=# select * from child1; > a > --- > (0 rows) > It is bug. Probably INHERITS clause is not well implemented for GTT > > postgres=# create temporary table parent2(a int) on commit delete rows; > CREATE TABLE > postgres=# create temporary table child2() inherits (parent2); > CREATE TABLE > postgres=# insert into parent2 values(1); > INSERT 0 1 > postgres=# insert into child2 values(2); > INSERT 0 1 > postgres=# select * from parent2; > a > --- > 2 > (1 row) > > postgres=# select * from child2; > a > --- > 2 > (1 row) > > > Thanks, > Prabhat Sahu > >
Re: [Proposal] Global temporary tables
Hi All, I observe a different behavior in "temporary table" and "global temporary table". Not sure if it is expected? postgres=# create global temporary table parent1(a int) on commit delete rows; CREATE TABLE postgres=# create global temporary table child1() inherits (parent1); CREATE TABLE postgres=# insert into parent1 values(1); INSERT 0 1 postgres=# insert into child1 values(2); INSERT 0 1 postgres=# select * from parent1; a --- (0 rows) postgres=# select * from child1; a --- (0 rows) postgres=# create temporary table parent2(a int) on commit delete rows; CREATE TABLE postgres=# create temporary table child2() inherits (parent2); CREATE TABLE postgres=# insert into parent2 values(1); INSERT 0 1 postgres=# insert into child2 values(2); INSERT 0 1 postgres=# select * from parent2; a --- 2 (1 row) postgres=# select * from child2; a --- 2 (1 row) Thanks, Prabhat Sahu
Re: Error on failed COMMIT
>> If we think the current *user-visible* behavior is problematic (commit on failed transaction completes without throwing), then the only remaining question is where this behavior should be fixed - at the server or at the driver. As I wrote above, from the user's perspective it makes no difference - the change would be identical (and just as breaking) either way. So while drivers *could* implement the new behavior, what advantages would that have over doing it at the server? Some disadvantages do seem clear (repetition of the logic across each driver - leading to inconsistency across drivers, changing semantics at the driver by turning a non-error into an exception...). > > The advantage is that it doesn't cause a compatibility break. I think it's very important to expand the reasoning here from "server and client" to "server, drivers, users". As I wrote above, changing this behavior in a driver is just as much a compatibility break for any user of that driver, as a server change; it's true that PostgreSQL would not be "responsible" ot "at fault" but rather the driver writer, but as far as angry users go there's very little difference. A break is a break, whether it happens because of a PostgreSQL change, or because of a .NET/Java driver change. > 2. It would be better to fix the driver than the server because this behavior is very old and there are probably many applications (and perhaps some drivers) that rely on it, and changing the server would break them. As above, if Dave and I make this change in the JDBC driver and/or Npgsql, all applications relying on the previous behavior would be just as broken. >> If we are assuming that most user code is already written to avoid committing on failed transactions (by tracking transaction state etc.), then making this change at the server wouldn't affect those applications; the only applications affected would be those that do commit on failed transactions today, and it could be argued that those are likely to be broken today (since drivers today don't really expose the rollback in an accessible/discoverable way). > > libpq exposes it just fine, so I think you're overgeneralizing here. The question is more whether typical user applications are actually checking for rollback-on-commit, not whether they theoretically can. An exception is something you have to actively swallow to ignore; an additional returned status saying "hey, this didn't actually commit" is extremely easy to ignore unless you've specifically been aware of the situation. Even so, a quick look at psycopg and Ruby (in addition to JDBC and .NET), commit APIs generally don't return anything - this is just how the API abstractions are, probably because across databases nothing like that is needed (the expectation is for a non-throwing commit to imply that the commit occurred). Shay On Mon, Feb 24, 2020 at 2:34 PM Robert Haas wrote: > On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky wrote: > > As Dave wrote, the problem here isn't with the driver, but with > framework or user-code which swallows the initial exception and allows code > to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does > surface PostgreSQL errors as exceptions, and internally tracks the > transaction status provided in the CommandComplete message. That means > users have the ability - but not the obligation - to know about failed > transactions, and some frameworks or user coding patterns could lead to a > commit being done on a failed transaction. > > Agreed. All of that can be fixed in the driver, though. > > > If we think the current *user-visible* behavior is problematic (commit > on failed transaction completes without throwing), then the only remaining > question is where this behavior should be fixed - at the server or at the > driver. As I wrote above, from the user's perspective it makes no > difference - the change would be identical (and just as breaking) either > way. So while drivers *could* implement the new behavior, what advantages > would that have over doing it at the server? Some disadvantages do seem > clear (repetition of the logic across each driver - leading to > inconsistency across drivers, changing semantics at the driver by turning a > non-error into an exception...). > > The advantage is that it doesn't cause a compatibility break. > > > > Well, it seems quite possible that there are drivers and applications > that don't have this issue; I've never had a problem with this behavior, > and I've been using PostgreSQL for something like two decades [...] > > > > If we are assuming that most user code is already written to avoid > committing on failed transactions (by tracking transaction state etc.), > then making this change at the server wouldn't affect those applications; > the only applications affected would be those that do commit on failed > transactions today, and it could be argued that those are likely to be > broken today (since drivers today don't really expose the
Re: Error on failed COMMIT
If we did change the server behavior, it seems unlikely that
> every driver would adjust their behavior to the new server behavior
> all at once and that they would all get it right while also all
> preserving backward compatibility with current releases in case a
> newer driver is used with an older server. I don't think that's
> likely. What would probably happen is that many drivers would ignore
> the change, leaving applications to cope with the differences between
> server versions, and some would change the driver behavior
> categorically, breaking compatibility with older server versions, and
> some would make mistakes in implementing support for the new behavior.
> And maybe we would also find that the new behavior isn't ideal for
> everybody any more than the current behavior is ideal for everybody.
>
To test how the driver would currently react if the server did respond with
an error I made a small change
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 0a6f80963b..9405b0cfd9 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2666,8 +2666,7 @@ IsTransactionExitStmt(Node *parsetree)
{
TransactionStmt *stmt = (TransactionStmt *) parsetree;
- if (stmt->kind == TRANS_STMT_COMMIT ||
- stmt->kind == TRANS_STMT_PREPARE ||
+ if (stmt->kind == TRANS_STMT_PREPARE ||
stmt->kind == TRANS_STMT_ROLLBACK ||
stmt->kind == TRANS_STMT_ROLLBACK_TO)
return true;
I have no idea how badly this breaks other things but it does throw an
error on commit if the transaction is in error.
With absolutely no changes to the driver this code does what I would expect
and executes the conn.rollback()
try {
conn.setAutoCommit(false);
try {
conn.createStatement().execute("insert into notnullable values (NULL)");
} catch (SQLException ex ) {
ex.printStackTrace();
//ignore this exception
}
conn.commit();
} catch ( SQLException ex ) {
ex.printStackTrace();
conn.rollback();
}
conn.close();
Dave Cramer
http://www.postgres.rocks
>
Re: Error on failed COMMIT
On Mon, 24 Feb 2020 at 07:34, Robert Haas wrote: > On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky wrote: > > As Dave wrote, the problem here isn't with the driver, but with > framework or user-code which swallows the initial exception and allows code > to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does > surface PostgreSQL errors as exceptions, and internally tracks the > transaction status provided in the CommandComplete message. That means > users have the ability - but not the obligation - to know about failed > transactions, and some frameworks or user coding patterns could lead to a > commit being done on a failed transaction. > > Agreed. All of that can be fixed in the driver, though. > Of course it can but we really don't want our users getting one experience with driver A and a different experience with driver B. > > > If we think the current *user-visible* behavior is problematic (commit > on failed transaction completes without throwing), then the only remaining > question is where this behavior should be fixed - at the server or at the > driver. As I wrote above, from the user's perspective it makes no > difference - the change would be identical (and just as breaking) either > way. So while drivers *could* implement the new behavior, what advantages > would that have over doing it at the server? Some disadvantages do seem > clear (repetition of the logic across each driver - leading to > inconsistency across drivers, changing semantics at the driver by turning a > non-error into an exception...). > > The advantage is that it doesn't cause a compatibility break. > Sure it does. Any existing code that was relying on the existing semantics would be incompatible. > > > > Well, it seems quite possible that there are drivers and applications > that don't have this issue; I've never had a problem with this behavior, > and I've been using PostgreSQL for something like two decades [...] > > > > If we are assuming that most user code is already written to avoid > committing on failed transactions (by tracking transaction state etc.), > then making this change at the server wouldn't affect those applications; > the only applications affected would be those that do commit on failed > transactions today, and it could be argued that those are likely to be > broken today (since drivers today don't really expose the rollback in an > accessible/discoverable way). > > libpq exposes it just fine, so I think you're overgeneralizing here. > > As I said upthread, I think one of the things that would be pretty > badly broken by this is psql -f something.sql, where something.sql > contains a series of blocks of the form "begin; something; something; > something; commit;". Right now whichever transactions succeed get > committed. With the proposed change, if one transaction block fails, > it'll merge with all of the following blocks. So how does one figure out what failed and what succeeded ? I would think it would be pretty difficult in a large sql script to go back and figure out what needed to be repaired. Seems to me it would be much easier if everything failed. > You may think that > nobody is doing this sort of thing, but I think people are, and that > they will come after us with pitchforks if we break it. > So the argument here is that we don't want to annoy some percentage of the population by doing the right thing ? Dave Cramer www.postgres.rocks
Re: v12 "won't fix" item regarding memory leak in "ATTACH PARTITION without AEL"; (or, relcache ref counting)
On Mon, Feb 24, 2020 at 7:01 AM Justin Pryzby wrote: > This links to a long thread, from which I've tried to quote some of the > most important mails, below. > https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Won.27t_Fix > > I wondered if there's an effort to pursue a resolution for v13 ? I think commit 5b9312378 in master branch fixes this. Commit message mentions it like this: ... Also, fix things so that old copies of a relcache partition descriptor will be dropped when the cache entry's refcount goes to zero. In the previous coding it was possible for such copies to survive for the lifetime of the session, as I'd complained of in a previous discussion. (This management technique still isn't perfect, but it's better than before.) ... ...Although this is a pre-existing problem, no back-patch: the patch seems too invasive to be safe to back-patch, and the bug it fixes is a corner case that seems relatively unlikely to cause problems in the field. Discussion: https://postgr.es/m/CA+HiwqFUzjfj9HEsJtYWcr1SgQ_=iCAvQ=o2sx6aqxodu4o...@mail.gmail.com Discussion: https://postgr.es/m/ca+tgmoy3brmgb6-dunovy5fjoreibj43rwmrqrcdpxukt4y...@mail.gmail.com Thanks, Amit
Re: Error on failed COMMIT
On Mon, 24 Feb 2020 at 07:25, Robert Haas wrote: > On Mon, Feb 24, 2020 at 7:29 AM Dave Cramer > wrote: > > Well the driver really isn't in the business of changing the semantics > of the server. > > I mean, I just can't agree with that way of characterizing it. It > seems clear enough that the driver not only should not change the > semantics of the server, but that it cannot. It can, however, decide > which of the things that the server might do (or that the application > connected to it might do) ought to result in it throwing an exception. > And a slightly different set of decisions here would produce the > desired behavior instead of behavior which is not desired. > > -- > Fair enough. What I meant to say was that the driver isn't in the business of providing different semantics than the server provides. > Dave Cramer > http://www.postgres.rocks >
Re: [PATCH] Erase the distinctClause if the result is unique by definition
Hi All: Here is the updated patch. It used some functions from query_is_distinct_for. I check the query's distinctness in create_distinct_paths, if it is distinct already, it will not generate the paths for that. so at last the query tree is not untouched. Please see if you have any comments. Thanks On Mon, Feb 24, 2020 at 8:38 PM Andy Fan wrote: > > > On Wed, Feb 12, 2020 at 12:36 AM Ashutosh Bapat < > ashutosh.bapat@gmail.com> wrote: > >> >> >> On Tue, Feb 11, 2020 at 8:27 AM Andy Fan >> wrote: >> >>> >>> >>> On Tue, Feb 11, 2020 at 12:22 AM Ashutosh Bapat < >>> ashutosh.bapat@gmail.com> wrote: >>> > > [PATCH] Erase the distinctClause if the result is unique by > definition > I forgot to mention this in the last round of comments. Your patch was actually removing distictClause from the Query structure. Please avoid doing that. If you remove it, you are also removing the evidence that this Query had a DISTINCT clause in it. >>> >>> Yes, I removed it because it is the easiest way to do it. what is the >>> purpose of keeping the evidence? >>> >> >> Julien's example provides an explanation for this. The Query structure is >> serialised into a view definition. Removing distinctClause from there means >> that the view will never try to produce unique results. >> >>> >>> >> > Actually it is not true. If a view is used in the query, the definition > will be *copied* > into the query tree. so if we modify the query tree, the definition of > the view never > touched. The issue of Julien reported is because of a typo error. > > -- session 2 >>> postgres=# alter table t alter column b drop not null; >>> ALTER TABLE >>> >>> -- session 1: >>> postgres=# explain execute st(1); >>> QUERY PLAN >>> - >>> Unique (cost=1.03..1.04 rows=1 width=4) >>>-> Sort (cost=1.03..1.04 rows=1 width=4) >>> Sort Key: b >>> -> Seq Scan on t (cost=0.00..1.02 rows=1 width=4) >>>Filter: (c = $1) >>> (5 rows) >>> >> >> Since this prepared statement is parameterised PostgreSQL is replanning >> it every time it gets executed. It's not using a stored prepared plan. Try >> without parameters. Also make sure that a prepared plan is used for >> execution and not a new plan. >> > > Even for parameterised prepared statement, it is still possible to > generate an generic > plan. so it will not replanning every time. But no matter generic plan or > not, after a DDL like > changing the NOT NULL constraints. pg will generated a plan based on the > stored query > tree. However, the query tree will be *copied* again to generate a new > plan. so even I > modified the query tree, everything will be ok as well. > > At last, I am agreed with that modifying the query tree is not a good > idea. > so my updated patch doesn't use it any more. > 0001-Erase-the-distinctClause-if-the-result-is-unique-by-.patch Description: Binary data
Re: Yet another vectorized engine
Hi Konstantin, I have added you as a collaborator on github. Please accepted and try again. I think non collaborator could also open pull requests. On Mon, Feb 24, 2020 at 8:02 PM Konstantin Knizhnik < k.knizh...@postgrespro.ru> wrote: > > > On 24.02.2020 05:08, Hubert Zhang wrote: > > Hi > > On Sat, Feb 22, 2020 at 12:58 AM Konstantin Knizhnik < > k.knizh...@postgrespro.ru> wrote: > >> >> >> On 12.02.2020 13:12, Hubert Zhang wrote: >> >> On Tue, Feb 11, 2020 at 1:20 AM Konstantin Knizhnik < >> k.knizh...@postgrespro.ru> wrote: >> >>> >>> So looks like PG-13 provides significant advantages in OLAP queries >>> comparing with 9.6! >>> Definitely it doesn't mean that vectorized executor is not needed for >>> new version of Postgres. >>> Once been ported, I expect that it should provide comparable >>> improvement of performance. >>> >>> But in any case I think that vectorized executor makes sense only been >>> combine with columnar store. >>> >> >> Thanks for the test. +1 on vectorize should be combine with columnar >> store. I think when we support this extension >> on master, we could try the new zedstore. >> I'm not active on this work now, but will continue when I have time. Feel >> free to join bring vops's feature into this extension. >> >> Thanks >> >> Hubert Zhang >> >> >> I have ported vectorize_engine to the master. >> It takes longer than I expected: a lot of things were changed in executor. >> >> Results are the following: >> >> >> par.warkers >> PG9_6 >> vectorize=off >> PG9_6 >> vectorize=on >> master >> vectorize=off >> jit=on >> master >> vectorize=off >> jit=off master >> vectorize=on >> jit=ofn master >> vectorize=on >> jit=off >> 0 >> 36 >> 20 >> 16 >> 25.5 >> 15 >> 17.5 >> 4 >> 10 >> - >> 5 7 >> - >> - >> >> So it proves the theory that JIT provides almost the same speedup as >> vector executor (both eliminates interpretation overhead but in different >> way). >> I still not sure that we need vectorized executor: because with standard >> heap it provides almost no improvements comparing with current JIT version. >> But in any case I am going to test it with vertical storage (zedstore or >> cstore). >> >> > Thanks for the porting and testing. > Yes, PG master and 9.6 have many changes, not only executor, but also > tupletableslot interface. > > What matters the performance of JIT and Vectorization is its > implementation. This is just the beginning of vectorization work, just as > your vops extension reported, vectorization could run 10 times faster in > PG. With the overhead of row storage(heap), we may not reach that speedup, > but I think we could do better. Also +1 on vertical storage. > > BTW, welcome to submit your PR for the PG master version. > > > > Sorry, but I have no permissions to push changes to your repository. > I can certainly create my own fork of vectorize_engine, but I think it > will be beter if I push pg13 branch in your repository. > > > -- Thanks Hubert Zhang
Re: [PATCH] Erase the distinctClause if the result is unique by definition
On Wed, Feb 12, 2020 at 12:36 AM Ashutosh Bapat < ashutosh.bapat@gmail.com> wrote: > > > On Tue, Feb 11, 2020 at 8:27 AM Andy Fan wrote: > >> >> >> On Tue, Feb 11, 2020 at 12:22 AM Ashutosh Bapat < >> ashutosh.bapat@gmail.com> wrote: >> >>> >>> [PATCH] Erase the distinctClause if the result is unique by definition >>> >>> I forgot to mention this in the last round of comments. Your patch was >>> actually removing distictClause from the Query structure. Please avoid >>> doing that. If you remove it, you are also removing the evidence that this >>> Query had a DISTINCT clause in it. >>> >> >> Yes, I removed it because it is the easiest way to do it. what is the >> purpose of keeping the evidence? >> > > Julien's example provides an explanation for this. The Query structure is > serialised into a view definition. Removing distinctClause from there means > that the view will never try to produce unique results. > >> >> > Actually it is not true. If a view is used in the query, the definition will be *copied* into the query tree. so if we modify the query tree, the definition of the view never touched. The issue of Julien reported is because of a typo error. -- session 2 >> postgres=# alter table t alter column b drop not null; >> ALTER TABLE >> >> -- session 1: >> postgres=# explain execute st(1); >> QUERY PLAN >> - >> Unique (cost=1.03..1.04 rows=1 width=4) >>-> Sort (cost=1.03..1.04 rows=1 width=4) >> Sort Key: b >> -> Seq Scan on t (cost=0.00..1.02 rows=1 width=4) >>Filter: (c = $1) >> (5 rows) >> > > Since this prepared statement is parameterised PostgreSQL is replanning it > every time it gets executed. It's not using a stored prepared plan. Try > without parameters. Also make sure that a prepared plan is used for > execution and not a new plan. > Even for parameterised prepared statement, it is still possible to generate an generic plan. so it will not replanning every time. But no matter generic plan or not, after a DDL like changing the NOT NULL constraints. pg will generated a plan based on the stored query tree. However, the query tree will be *copied* again to generate a new plan. so even I modified the query tree, everything will be ok as well. At last, I am agreed with that modifying the query tree is not a good idea. so my updated patch doesn't use it any more.
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky wrote: > As Dave wrote, the problem here isn't with the driver, but with framework or > user-code which swallows the initial exception and allows code to continue to > the commit. Npgsql (and I'm sure the JDBC driver too) does surface PostgreSQL > errors as exceptions, and internally tracks the transaction status provided > in the CommandComplete message. That means users have the ability - but not > the obligation - to know about failed transactions, and some frameworks or > user coding patterns could lead to a commit being done on a failed > transaction. Agreed. All of that can be fixed in the driver, though. > If we think the current *user-visible* behavior is problematic (commit on > failed transaction completes without throwing), then the only remaining > question is where this behavior should be fixed - at the server or at the > driver. As I wrote above, from the user's perspective it makes no difference > - the change would be identical (and just as breaking) either way. So while > drivers *could* implement the new behavior, what advantages would that have > over doing it at the server? Some disadvantages do seem clear (repetition of > the logic across each driver - leading to inconsistency across drivers, > changing semantics at the driver by turning a non-error into an exception...). The advantage is that it doesn't cause a compatibility break. > > Well, it seems quite possible that there are drivers and applications that > > don't have this issue; I've never had a problem with this behavior, and > > I've been using PostgreSQL for something like two decades [...] > > If we are assuming that most user code is already written to avoid committing > on failed transactions (by tracking transaction state etc.), then making this > change at the server wouldn't affect those applications; the only > applications affected would be those that do commit on failed transactions > today, and it could be argued that those are likely to be broken today (since > drivers today don't really expose the rollback in an accessible/discoverable > way). libpq exposes it just fine, so I think you're overgeneralizing here. As I said upthread, I think one of the things that would be pretty badly broken by this is psql -f something.sql, where something.sql contains a series of blocks of the form "begin; something; something; something; commit;". Right now whichever transactions succeed get committed. With the proposed change, if one transaction block fails, it'll merge with all of the following blocks. You may think that nobody is doing this sort of thing, but I think people are, and that they will come after us with pitchforks if we break it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 1:31 PM Vik Fearing wrote: > On 24/02/2020 02:31, Robert Haas wrote: > > I am really struggling to see why this is anything but a bug in the > > JDBC driver. > > I can follow your logic for it being a bug in the JDBC driver, but > "anything but"? No, this is (also) an undocumented violation of SQL. Well, that's a fair point. I withdraw my previous statement. Instead, I wish to argue that: 1. This problem can definitely be fixed in any given driver without changing the behavior of the server. 2. It would be better to fix the driver than the server because this behavior is very old and there are probably many applications (and perhaps some drivers) that rely on it, and changing the server would break them. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [Patch] Make pg_checksums skip foreign tablespace directories
On Sun, Feb 23, 2020 at 04:08:58PM +0900, Michael Paquier wrote: > Good idea. Let's do things as you suggest. Applied and back-patched this one down to 11. -- Michael signature.asc Description: PGP signature
Re: Error on failed COMMIT
On Mon, Feb 24, 2020 at 7:29 AM Dave Cramer wrote: > Well the driver really isn't in the business of changing the semantics of the > server. I mean, I just can't agree with that way of characterizing it. It seems clear enough that the driver not only should not change the semantics of the server, but that it cannot. It can, however, decide which of the things that the server might do (or that the application connected to it might do) ought to result in it throwing an exception. And a slightly different set of decisions here would produce the desired behavior instead of behavior which is not desired. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [Patch] Make pg_checksums skip foreign tablespace directories
On Fri, 2020-02-21 at 15:36 +0900, Michael Paquier wrote: > We don't do that for the normal directory scan path, so it does not > strike me as a good idea on consistency ground. As a whole, I don't > see much point in having a separate routine which is just roughly a > duplicate of scan_directory(), and I think that we had better just > add > the check looking for matches with TABLESPACE_VERSION_DIRECTORY > directly when having a directory, if subdir is "pg_tblspc". That > also makes the patch much shorter. To be honest, i dislike both: The other doubles logic (note: i don't see it necessarily as 100% code duplication, since the semantic of scan_tablespaces() is different: it serves as a driver for scan_directories() and just resolves entries in pg_tblspc directly). The other makes scan_directories() complicater to read and special cases just a single directory in an otherwise more or less generic function. E.g. it makes me uncomfortable if we get a pg_tblspc somewhere else than PGDATA (if someone managed to create such a directory in a foreign tablespace location for example), so we should maintain an additional check if we really operate on the pg_tblspc we have to. That was the reason(s) i've moved it into a separate function. That said, i'll provide an updated patch with your ideas. Bernd
Re: Yet another vectorized engine
On 24.02.2020 05:08, Hubert Zhang wrote: Hi On Sat, Feb 22, 2020 at 12:58 AM Konstantin Knizhnik mailto:k.knizh...@postgrespro.ru>> wrote: On 12.02.2020 13:12, Hubert Zhang wrote: On Tue, Feb 11, 2020 at 1:20 AM Konstantin Knizhnik mailto:k.knizh...@postgrespro.ru>> wrote: So looks like PG-13 provides significant advantages in OLAP queries comparing with 9.6! Definitely it doesn't mean that vectorized executor is not needed for new version of Postgres. Once been ported, I expect that it should provide comparable improvement of performance. But in any case I think that vectorized executor makes sense only been combine with columnar store. Thanks for the test. +1 on vectorize should be combine with columnar store. I think when we support this extension on master, we could try the new zedstore. I'm not active on this work now, but will continue when I have time. Feel free to join bring vops's feature into this extension. Thanks Hubert Zhang I have ported vectorize_engine to the master. It takes longer than I expected: a lot of things were changed in executor. Results are the following: par.warkers PG9_6 vectorize=off PG9_6 vectorize=on master vectorize=off jit=on master vectorize=off jit=off master vectorize=on jit=ofn master vectorize=on jit=off 0 36 20 16 25.5 15 17.5 4 10 - 5 7 - - So it proves the theory that JIT provides almost the same speedup as vector executor (both eliminates interpretation overhead but in different way). I still not sure that we need vectorized executor: because with standard heap it provides almost no improvements comparing with current JIT version. But in any case I am going to test it with vertical storage (zedstore or cstore). Thanks for the porting and testing. Yes, PG master and 9.6 have many changes, not only executor, but also tupletableslot interface. What matters the performance of JIT and Vectorization is its implementation. This is just the beginning of vectorization work, just as your vops extension reported, vectorization could run 10 times faster in PG. With the overhead of row storage(heap), we may not reach that speedup, but I think we could do better. Also +1 on vertical storage. BTW, welcome to submit your PR for the PG master version. Sorry, but I have no permissions to push changes to your repository. I can certainly create my own fork of vectorize_engine, but I think it will be beter if I push pg13 branch in your repository.
Re: allow frontend use of the backend's core hashing functions
On Fri, Feb 14, 2020 at 9:03 PM Robert Haas wrote: > On Thu, Feb 13, 2020 at 11:26 AM Mark Dilger > wrote: > > I have made these changes and rebased Robert’s patches but otherwise > > changed nothing. Here they are: > > Thanks. Anyone else have comments? I think this is pretty > straightforward and unobjectionable work so I'm inclined to press > forward with committing it fairly soon, but if someone feels > otherwise, please speak up. I've committed 0001 through 0003 as revised by Mark in accordance with the comments from Suraj. Here's the last patch again with a tweak to try not to break the Windows build, per some off-list advice I received on how not to break the Windows build. Barring complaints from the buildfarm or otherwise, I'll commit this one too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company v3-0001-Move-src-backend-utils-hash-hashfn.c-to-src-commo.patch Description: Binary data
Re: ALTER TABLE ... SET STORAGE does not propagate to indexes
Hello Peter,
On Mon, Feb 24, 2020 at 12:59 PM Peter Eisentraut
wrote:
>
> On 2020-01-06 13:32, Peter Eisentraut wrote:
> > Attached is a patch that attempts to fix this by propagating the storage
> > change to existing indexes. This triggers a few regression test
> > failures (going from no error to error), which I attempted to fix up,
> > but I haven't analyzed what the tests were trying to do, so it might
> > need another look.
>
> Attached is a more polished patch, with tests.
I've reproduced the issue on head. And, the patch seems to solve the
problem. The patch looks good to me. But, I've a small doubt regarding
the changes in test_decoding regression file.
diff --git a/contrib/test_decoding/sql/toast.sql
b/contrib/test_decoding/sql/toast.sql
..
-INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 1));
-SELECT pg_column_size(toasted_key) > 2^16 FROM toasted_several;
+INSERT INTO toasted_several(toasted_key) VALUES(repeat('9876543210', 269));
+SELECT pg_column_size(toasted_key) > 2^11 FROM toasted_several;
This actually tests whether we can decode "old" tuples bigger than the
max heap tuple size correctly which is around 8KB. But, the above
changes will make the tuple size around 3KB. So, it'll not be able to
test that particular scenario.Thoughts?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
Re: Parallel grouping sets
To summarize the current state of parallel grouping sets, we now have two available implementations for it. 1) Each worker performs an aggregation step, producing a partial result for each group of which that process is aware. Then the partial results are gathered to the leader, which then performs a grouping sets aggregation, as in patch [1]. This implementation is not very efficient sometimes, because the group key for Partial Aggregate has to be all the columns involved in the grouping sets. 2) Each worker performs a grouping sets aggregation on its partial data, and tags 'GroupingSetId' for each tuple produced by partial aggregate. Then the partial results are gathered to the leader, and the leader performs a modified grouping aggregate, which dispatches the partial results into different pipe according to 'GroupingSetId', as in patch [2], or instead as another method, the leader performs a normal aggregation, with 'GroupingSetId' included in the group keys, as discussed in [3]. The second implementation would be generally better than the first one in performance, and we have decided to concentrate on it. [1] https://www.postgresql.org/message-id/CAN_9JTx3NM12ZDzEYcOVLFiCBvwMHyM0gENvtTpKBoOOgcs=k...@mail.gmail.com [2] https://www.postgresql.org/message-id/can_9jtwtttnxhbr5ahuqvcriz3hxvppx1jwe--dcsdjyuhr...@mail.gmail.com [3] https://www.postgresql.org/message-id/CAN_9JTwtzttEmdXvMbJqXt=51kxibtckepkq6kk2pz6xz6m...@mail.gmail.com Thanks Richard >
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
On Thu, Feb 20, 2020 at 8:06 AM Andres Freund wrote: > > Hi, > > On 2020-02-19 11:12:18 +0530, Amit Kapila wrote: > > I think till we know the real need for changing group locking, going > > in the direction of what Tom suggested to use an array of LWLocks [1] > > to address the problems in hand is a good idea. > > -many > > I think that building yet another locking subsystem is the entirely > wrong idea - especially when there's imo no convincing architectural > reasons to do so. > Hmm, AFAIU, it will be done by having an array of LWLocks which we do at other places as well (like BufferIO locks). I am not sure if we can call it as new locking subsystem, but if we decide to continue using lock.c and change group locking then I think we can do that as well, see my comments below regarding that. > > > It is not very clear to me that are we thinking to give up on Tom's > > idea [1] and change group locking even though it is not clear or at > > least nobody has proposed an idea/patch which requires that? Or are > > we thinking that we can do what Tom suggested for relation extension > > lock and also plan to change group locking for future parallel > > operations that might require it? > > What I'm advocating is that extension locks should continue to go > through lock.c. And yes, that requires some changes to group locking, > but I still don't see why they'd be complicated. > Fair position, as per initial analysis, I think if we do below three things, it should work out without changing to a new way of locking for relation extension or page type locks. a. As per the discussion above, ensure in code we will never try to acquire another heavy-weight lock after acquiring relation extension or page type locks (probably by having Asserts in code or maybe some other way). b. Change lock.c so that group locking is not considered for these two lock types. For ex. in LockCheckConflicts, along with the check (if (proclock->groupLeader == MyProc && MyProc->lockGroupLeader == NULL)), we also check lock->tag and call it a conflict for these two locks. c. The deadlock detector can ignore checking these two types of locks because point (a) ensures that those won't lead to deadlock. One idea could be that FindLockCycleRecurseMember just ignores these two types of locks by checking the lock tag. It is possible that I might be missing something or we could achieve this some other way as well. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
allow trigger to get updated columns
This is a change to make the bitmap of updated columns available to a
trigger in TriggerData. This is the same idea as was recently done to
generated columns [0]: Generic triggers such as tsvector_update_trigger
can use this information to skip work if the columns they are interested
in haven't changed. With the generated columns change, perhaps this
isn't so interesting anymore, but I suspect a lot of existing
installations still use tsvector_update_trigger. In any case, since I
had already written the code, I figured I post it here. Perhaps there
are other use cases.
[0]:
https://www.postgresql.org/message-id/flat/b05e781a-fa16-6b52-6738-761181204...@2ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 0c901266a1b40a257320166fdacaaefd00e4dbce Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 24 Feb 2020 10:12:10 +0100
Subject: [PATCH 1/2] Code simplification
Initialize TriggerData to 0 for the whole struct together, instead of
each field separately.
---
src/backend/commands/tablecmds.c | 4 +-
src/backend/commands/trigger.c | 78 +---
2 files changed, 12 insertions(+), 70 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b7c8d663fc..6af984ff10 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10264,7 +10264,7 @@ validateForeignKeyConstraint(char *conname,
while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
{
LOCAL_FCINFO(fcinfo, 0);
- TriggerData trigdata;
+ TriggerData trigdata = {0};
CHECK_FOR_INTERRUPTS();
@@ -10283,8 +10283,6 @@ validateForeignKeyConstraint(char *conname,
trigdata.tg_relation = rel;
trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false,
NULL);
trigdata.tg_trigslot = slot;
- trigdata.tg_newtuple = NULL;
- trigdata.tg_newslot = NULL;
trigdata.tg_trigger =
fcinfo->context = (Node *)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b9b1262e30..26593576fd 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2458,7 +2458,7 @@ ExecBSInsertTriggers(EState *estate, ResultRelInfo
*relinfo)
{
TriggerDesc *trigdesc;
int i;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
trigdesc = relinfo->ri_TrigDesc;
@@ -2476,12 +2476,6 @@ ExecBSInsertTriggers(EState *estate, ResultRelInfo
*relinfo)
LocTriggerData.tg_event = TRIGGER_EVENT_INSERT |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger*trigger = >triggers[i];
@@ -2528,7 +2522,7 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple newtuple = NULL;
boolshould_free;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -2536,12 +2530,6 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_BEFORE;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
- LocTriggerData.tg_newslot = NULL;
- LocTriggerData.tg_oldtable = NULL;
- LocTriggerData.tg_newtable = NULL;
for (i = 0; i < trigdesc->numtriggers; i++)
{
Trigger*trigger = >triggers[i];
@@ -2610,7 +2598,7 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
HeapTuple newtuple = NULL;
boolshould_free;
- TriggerData LocTriggerData;
+ TriggerData LocTriggerData = {0};
int i;
LocTriggerData.type = T_TriggerData;
@@ -2618,12 +2606,6 @@ ExecIRInsertTriggers(EState *estate, ResultRelInfo
*relinfo,
TRIGGER_EVENT_ROW |
TRIGGER_EVENT_INSTEAD;
LocTriggerData.tg_relation = relinfo->ri_RelationDesc;
- LocTriggerData.tg_trigtuple = NULL;
- LocTriggerData.tg_newtuple = NULL;
- LocTriggerData.tg_trigslot = NULL;
-
Re: [Proposal] Global temporary tables
On Fri, Feb 21, 2020 at 9:10 PM 曾文旌(义从) wrote: > Hi, > I have started testing the "Global temporary table" feature, > That's great, I see hope. > from "gtt_v11-pg13.patch". Below is my findings: > > -- session 1: > postgres=# create global temporary table gtt1(a int); > CREATE TABLE > > -- seeeion 2: > postgres=# truncate gtt1 ; > ERROR: could not open file "base/13585/t3_16384": No such file or > directory > > is it expected? > > Oh ,this is a bug, I fixed it. > Thanks for the patch. I have verified the same, Now the issue is resolved with v12 patch. Kindly confirm the below scenario: postgres=# create global temporary table gtt1 (c1 int unique); CREATE TABLE postgres=# create global temporary table gtt2 (c1 int references gtt1(c1) ); ERROR: referenced relation "gtt1" is not a global temp table postgres=# create table tab2 (c1 int references gtt1(c1) ); ERROR: referenced relation "gtt1" is not a global temp table Thanks, Prabhat Sahu
Re: Make java client lib accept same connection strings as psql
On Sat, Feb 22, 2020 at 4:05 AM David G. Johnston < david.g.johns...@gmail.com> wrote: > On Fri, Feb 21, 2020 at 6:21 PM Michael Leonhard > wrote: > >> How about making the Java client library accept the same connection >> strings as psql and other command-line tools? [...] >> > > That falls outside the scope of this list/project. The separate pgJDBC > project team would need to decide to take that up. > Michael, While your proposed end goal sounds as a desirable thing to me, there are certain obstacles to that, unfortunately. First, consider that the URL support appeared in libpq after, not before the support in JDBC driver. Second, the common subset of allowed connection parameters between the two is only as big as "host", "port", "database", "user" and "password". Additionally, libpq understands the "ssl=true" parameter for JDBC compatibility, though I don't think that was a good idea in the end. For one, in the JDBC world "ssl=false" is treated exactly the same as "ssl=true" or any other value, which is questionable design in the first place. And even if you could use exactly the same URL in both libpq-based and JDBC clients, without running into syntax errors, the semantics of "ssl=true" is subtly different between the two: in the former case, the client does not attempt to validate certificate, nor the hostname, as opposed to the latter. As to your actual example, the part of syntax that is treated differently in libpq is the the "userinfo": https://tools.ietf.org/html/rfc3986#section-3.2.1 The JDBC driver could be extended to support this as well, as such a change is backwards-compatible. As David has pointed out, this question should be asked to the PgJDBC project. Lastly, the RFC provides some good arguments as to why providing username and, especially, password in the connection URL is undesirable. While the "user:password@host" or "?user=fred=secret" syntax can be handy for local testing, this is definitely not something that should be used in production. Both libpq and the JDBC driver provide ways to pass login credentials in a more secure manner. Kind regards, -- Alex
Re: Yet another fast GiST build
Hi Thomas! Thanks for looking into this! I’ll fix your notices asap. > On 24 февр. 2020 г., at 01:58, Thomas Munro wrote: > > On Thu, Feb 20, 2020 at 10:14 AM Thomas Munro wrote: >> 1. We expect floats to be in IEEE format, and the sort order of IEEE >> floats is mostly correlated to the binary sort order of the bits >> reinterpreted as an int. It isn't in some special cases, but for this >> use case we don't really care about that, we're just trying to >> encourage locality. > > I suppose there is a big jump in integer value (whether signed or > unsigned) as you cross from positive to negative floats, and then the > sort order is reversed. I have no idea if either of those things is a > problem worth fixing. That made me wonder if there might also be an > endianness problem. It seems from some quick googling that all > current architectures have integers and floats of the same endianness. > Apparently this wasn't always the case, and some ARMs have a weird > half-flipped arrangement for 64 bit floats, but not 32 bit floats as > you are using here. Yes, this leap is a problem for point as generic data type. And I do not know how to fix it. It can cause inefficient Index Scans when searching near (0,0) and query window touches simultaneously all quadrants (4x slower). But everything will be just fine when all data is in 2nd quadrant. Actually, we do not need to add this hacky code to core: we can provide colum-wise ordering or something similar as an example. This feature is aimed at PostGIS and they already possess bit tricks tricks [0]. I’ve taken this union code from PostGIS. Thanks! Best regards, Andrey Borodin. [0] https://github.com/postgis/postgis/blob/master/postgis/gserialized_gist_nd.c#L1150
Re: Error on failed COMMIT
> First, to repeat what I said before, the COMMIT only returns a ROLLBACK
command tag if there's been a previous ERROR. So, if you haven't ignored
the prior ERROR, you should be fine. [...]
> I am really struggling to see why this is anything but a bug in the JDBC
driver
As Dave wrote, the problem here isn't with the driver, but with framework
or user-code which swallows the initial exception and allows code to
continue to the commit. Npgsql (and I'm sure the JDBC driver too) does
surface PostgreSQL errors as exceptions, and internally tracks the
transaction status provided in the CommandComplete message. That means
users have the ability - but not the obligation - to know about failed
transactions, and some frameworks or user coding patterns could lead to a
commit being done on a failed transaction.
> So, if you haven't ignored the prior ERROR, you should be fine. Second,
there's nothing to keep the driver itself from translating ROLLBACK into an
exception, if that's more convenient for some particular driver. [...]
This is the main point here IMHO, and I don't think it's a question of
convenience, or of behavior that should vary across drivers.
If we think the current *user-visible* behavior is problematic (commit on
failed transaction completes without throwing), then the only remaining
question is where this behavior should be fixed - at the server or at the
driver. As I wrote above, from the user's perspective it makes no
difference - the change would be identical (and just as breaking) either
way. So while drivers *could* implement the new behavior, what advantages
would that have over doing it at the server? Some disadvantages do seem
clear (repetition of the logic across each driver - leading to
inconsistency across drivers, changing semantics at the driver by turning a
non-error into an exception...).
> Well, it seems quite possible that there are drivers and applications
that don't have this issue; I've never had a problem with this behavior,
and I've been using PostgreSQL for something like two decades [...]
If we are assuming that most user code is already written to avoid
committing on failed transactions (by tracking transaction state etc.),
then making this change at the server wouldn't affect those applications;
the only applications affected would be those that do commit on failed
transactions today, and it could be argued that those are likely to be
broken today (since drivers today don't really expose the rollback in an
accessible/discoverable way).
Shay
On Mon, Feb 24, 2020 at 3:31 AM Robert Haas wrote:
> On Sun, Feb 23, 2020 at 11:11 AM Shay Rojansky wrote:
> > I'd like to second Dave on this, from the .NET perspective - actual
> client access is done via standard drivers in almost all cases, and these
> drivers generally adhere to database API abstractions (JDBC for Java,
> ADO.NET for .NET, and so on). AFAIK, in almost all such abstractions,
> commit can either complete (implying success) or throw an exception - there
> is no third way to return a status code. It's true that a driver may expose
> NOTICE/WARNING messages via some other channel (Npgsql emits .NET events
> for these), but this is a separate message "channel" that is disconnected
> API-wise from the commit; this makes the mechanism very "undiscoverable".
>
> I'm still befuddled here. First, to repeat what I said before, the
> COMMIT only returns a ROLLBACK command tag if there's been a previous
> ERROR. So, if you haven't ignored the prior ERROR, you should be fine.
> Second, there's nothing to keep the driver itself from translating
> ROLLBACK into an exception, if that's more convenient for some
> particular driver. Let's go back to Bernhard's example upthred:
>
> composeTransaction() {
> Connection con = getConnection(); // implicitly "begin"
> try {
>insertFrameworkLevelState(con);
>insertApplicationLevelState(con);
>con.commit();
>publishNewState();
> } catch (Throwable ex) {
>con.rollback();
> }
> }
>
> If insertFrameworkLevelState() or insertApplicationLevelState()
> perform database operations that fail, then an exception should be
> thrown and we should end up at con.rollback(), unless there is an
> internal catch block inside those functions that swallows the
> exception, or unless the JDBC driver ignores the error from the
> server. If those things succeed, then COMMIT could still fail with an
> ERROR but it shouldn't return ROLLBACK. But, for extra security,
> con.commit() could be made to throw an exception if the command tag
> returned by COMMIT is not COMMIT. It sounds like Dave doesn't want to
> do that, but it would solve this problem without requiring a server
> behavior change.
>
> Actually, an even better idea might be to make the driver error out
> when the transaction is known to be in a failed state when you enter
> con.commit(). The server does return an indication after each command
> as to whether the session is in a transaction
Remove win32ver.rc from version_stamp.pl
This removes another relic from the old nmake-based Windows build.
version_stamp.pl put version number information into win32ver.rc. But
win32ver.rc already gets other version number information from the
preprocessor at build time, so it would make more sense if all version
number information would be handled in the same way and we don't have
two places that do it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b14ec81c4dd7ee24875aa2b26293ec55237a22d9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 24 Feb 2020 08:46:13 +0100
Subject: [PATCH] Remove win32ver.rc from version_stamp.pl
This removes another relic from the old nmake-based Windows build.
version_stamp.pl put version number information into win32ver.rc. But
win32ver.rc already gets other version number information from the
preprocessor at build time, so it would make more sense if all version
number information would be handled in the same way and we don't have
two places that do it.
What we need for this is having the major version number and the minor
version number as separate symbols. Both configure and Solution.pm
already have that logic, because they compute PG_VERSION_NUM. So we
just keep all the logic there now. Put the minor version number into
a new symbol PG_MINORVERSION_NUM. For consistency, replace
PG_MAJORVERSION, which is a string, with PG_MAJORVERSION_NUM, which is
an integer.
---
configure | 12 +---
configure.in | 8 +---
src/bin/initdb/initdb.c | 6 +++---
src/bin/pg_resetwal/pg_resetwal.c | 12 ++--
src/bin/pg_upgrade/check.c| 4 ++--
src/bin/psql/help.c | 2 +-
src/bin/psql/startup.c| 2 +-
src/include/c.h | 4 ++--
src/include/common/relpath.h | 2 +-
src/include/pg_config.h.in| 7 +--
src/port/win32ver.rc | 4 ++--
src/tools/msvc/Install.pm | 2 +-
src/tools/msvc/Solution.pm| 16
src/tools/version_stamp.pl| 14 +-
14 files changed, 47 insertions(+), 48 deletions(-)
diff --git a/configure b/configure
index d2c74e530c..ce6ff9608e 100755
--- a/configure
+++ b/configure
@@ -2804,10 +2804,17 @@ _ACEOF
PG_MAJORVERSION=`expr "$PACKAGE_VERSION" : '\([0-9][0-9]*\)'`
+PG_MINORVERSION=`expr "$PACKAGE_VERSION" : '.*\.\([0-9][0-9]*\)'`
+test -n "$PG_MINORVERSION" || PG_MINORVERSION=0
cat >>confdefs.h <<_ACEOF
-#define PG_MAJORVERSION "$PG_MAJORVERSION"
+#define PG_MAJORVERSION_NUM $PG_MAJORVERSION
+_ACEOF
+
+
+cat >>confdefs.h <<_ACEOF
+#define PG_MINORVERSION_NUM $PG_MINORVERSION
_ACEOF
@@ -18861,8 +18868,7 @@ _ACEOF
# Supply a numeric version string for use by 3rd party add-ons
# awk -F is a regex on some platforms, and not on others, so make "." a tab
-PG_VERSION_NUM="`echo "$PACKAGE_VERSION" | sed 's/[A-Za-z].*$//' |
-tr '.' ' ' |
+PG_VERSION_NUM="`echo "$PG_MAJORVERSION$PG_MINORVERSION" |
$AWK '{printf "%d%04d", $1, $2}'`"
cat >>confdefs.h <<_ACEOF
diff --git a/configure.in b/configure.in
index 0b0a871298..47cd232bf8 100644
--- a/configure.in
+++ b/configure.in
@@ -30,8 +30,11 @@ AC_PREFIX_DEFAULT(/usr/local/pgsql)
AC_DEFINE_UNQUOTED(CONFIGURE_ARGS, ["$ac_configure_args"], [Saved arguments
from configure])
[PG_MAJORVERSION=`expr "$PACKAGE_VERSION" : '\([0-9][0-9]*\)'`]
+[PG_MINORVERSION=`expr "$PACKAGE_VERSION" : '.*\.\([0-9][0-9]*\)'`]
+test -n "$PG_MINORVERSION" || PG_MINORVERSION=0
AC_SUBST(PG_MAJORVERSION)
-AC_DEFINE_UNQUOTED(PG_MAJORVERSION, "$PG_MAJORVERSION", [PostgreSQL major
version as a string])
+AC_DEFINE_UNQUOTED(PG_MAJORVERSION_NUM, $PG_MAJORVERSION, [PostgreSQL major
version number])
+AC_DEFINE_UNQUOTED(PG_MINORVERSION_NUM, $PG_MINORVERSION, [PostgreSQL minor
version number])
PGAC_ARG_REQ(with, extra-version, [STRING], [append STRING to version],
[PG_VERSION="$PACKAGE_VERSION$withval"],
@@ -2317,8 +2320,7 @@ AC_DEFINE_UNQUOTED(PG_VERSION_STR,
# Supply a numeric version string for use by 3rd party add-ons
# awk -F is a regex on some platforms, and not on others, so make "." a tab
-[PG_VERSION_NUM="`echo "$PACKAGE_VERSION" | sed 's/[A-Za-z].*$//' |
-tr '.' ' ' |
+[PG_VERSION_NUM="`echo "$PG_MAJORVERSION $PG_MINORVERSION" |
$AWK '{printf "%d%04d", $1, $2}'`"]
AC_DEFINE_UNQUOTED(PG_VERSION_NUM, $PG_VERSION_NUM, [PostgreSQL version as a
number])
AC_SUBST(PG_VERSION_NUM)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 5302973379..acdacd05f5 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -846,7 +846,7 @@ write_version_file(const char *extrapath)
pg_log_error("could not open file \"%s\" for writing: %m",
path);
exit(1);
}
- if (fprintf(version_file, "%s\n", PG_MAJORVERSION) < 0 ||
+ if (fprintf(version_file, "%d\n",
Re: Error on failed COMMIT
On 24/02/2020 02:31, Robert Haas wrote: > I am really struggling to see why this is anything but a bug in the > JDBC driver. I can follow your logic for it being a bug in the JDBC driver, but "anything but"? No, this is (also) an undocumented violation of SQL. -- Vik Fearing
