RE: speeding up planning with partitions
Tsunakawa-san Thanks for giving the information. I didn't use it yet, but I just used perf to clarify the difference of before and after the creation of the generic plan, and I noticed that usage of hash_seq_search() is increased about 3% in EXECUTE queries after the creation of the generic plan. What I understand so far is about 10,000 while loops at total (4098+4098+some extra) is needed in hash_seq_search() in EXECUTE query after the creation of the generic plan. 10,000 while loops takes about 10 microsec (of course, we can't estimate correct time), and the difference of the latency between 5th and 7th EXECUTE is about 8 microsec, I currently think this causes the difference. I don't know this problem relates to Amit-san's patch, but I'll continue to investigate it. Yoshikazu Imai
RE: Thread-unsafe coding in ecpg
From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Ouch, thanks. And I'm sorry to annoy you by pointing out a trivial thing: in
v3 patch, _configthreadlocale() is not called to restore the original value
when setlocale() or ecpg_strdup() fails. I hope this is fixed in v4.
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
Regards
Takayuki Tsunakawa
RE: Protect syscache from bloating with negative cache entries
From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@lab.ntt.co.jp] > Although this doesn't put a hard cap on memory usage, it is indirectly and > softly limited by the cache_prune_min_age and cache_memory_target, which > determins how large a cache can grow until pruning happens. They are > per-cache basis. > > If we prefer to set a budget on all the syschaches (or even including other > caches), it would be more complex. > This is a pure question. How can we answer these questions from users? * What value can I set to cache_memory_target when I can use 10 GB for the caches and max_connections = 100? * How much RAM do I need to have for the caches when I set cache_memory_target = 1M? The user tends to estimate memory to avoid OOM. Regards Takayuki Tsunakawa
Re: [HACKERS] REINDEX CONCURRENTLY 2.0
On Sat, Jan 19, 2019 at 03:01:07AM +0100, Vik Fearing wrote: > On 19/01/2019 02:33, Michael Paquier wrote: >> On Fri, Jan 18, 2019 at 07:58:06PM +0100, Vik Fearing wrote: >>> My vote is to have homogeneous syntax for all of this, and so put it in >>> parentheses, but we should also allow CREATE INDEX and DROP INDEX to use >>> parentheses for it, too. >> >> That would be a new thing as these variants don't exist yet, and WITH >> is for storage parameters. In my opinion, the long-term take on doing >> such things is that we are then able to reduce the number of reserved >> keywords in the grammar. Even if for the case of CONCURRENTLY we may >> see humans on Mars before this actually happens, this does not mean >> that we should not do it moving forward for other keywords in the >> grammar. > > I'm not sure I understand your point. > > I don't want a situation like this: > CREATE INDEX CONCURRENTLY ... > DROP INDEX CONCURRENTLY ... > REINDEX INDEX (CONCURRENTLY) ... > > All three should be the same, and my suggestion is to add the > parenthesized version to CREATE and DROP and not add the unparenthesized > version to REINDEX. I am not sure what is the actual reason which could force to decide that all three queries should have the same grammar, and why this has anything to do on a thread about REINDEX. REINDEX can work on many more object types than an index so its scope is much larger, contrary to CREATE/DROP INDEX. An advantage of using parenthesized grammar and prioritize it is that you don't have to add it to the list of reserved keywords, and the parser can rely on IDENT for its work. I personally prefer the parenthesized grammar for that reason. If the crowd votes in majority for the other option, that's of course fine to me too. > I never said anything about WITH. Perhaps I have not explained my thoughts clearly here. My point was that if some day we decide to drop the non-parenthesized grammar of CREATE/DROP INDEX, one possibility would be to have a "concurrent" option as part of WITH, even if that's used only now for storage parameters. That's the only actual part of the grammar which is extensible. -- Michael signature.asc Description: PGP signature
Re: [PATCH] Pass COPT and PROFILE to CXXFLAGS as well
On Thu, Jan 17, 2019 at 02:55:39PM +0900, Michael Paquier wrote: > Personally I see pgxs as something completely different than what COPT > and PROFILE are as we are talking about two different facilities: one > which is part of the core installation, and the other which can be > used for extension modules, so having PG_CFLAGS, PG_CXXFLAGS and > PG_LDFLAGS, but leaving CXXFLAGS out of COPT and PROFILE looks like > the better long-term move in terms of pluggability. My 2c. It's been a couple of days since this message, and while my opinion has not really changed, there are many other opinions. So perhaps we could reduce the proposal to a strict minimum and find an agreement for the options that we think are absolutely worth adding? Even if we cannot agree on what COPT of PROFILE should do more, perhaps we could still agree with only a portion of the flags we think are worth it? -- Michael signature.asc Description: PGP signature
Re: A few new options for vacuumdb
On Mon, Jan 21, 2019 at 10:09:09PM +, Bossart, Nathan wrote: > Here's a new patch set that should address the feedback in this > thread. The changes in this version include: > > - 0001 is a small fix to the 'vacuumdb --disable-page-skipping' >documentation. My suggestion is to keep it short and simple like >--full, --freeze, --skip-locked, --verbose, and --analyze. The >DISABLE_PAGE_SKIPPING option is well-described in the VACUUM >documentation, and IMO it is reasonably obvious that such vacuumdb >options correspond to the VACUUM options. Okay, committed this one. > - v3-0002 is essentially v2-0001 and v2-0004 combined. I've also >added a comment explaining the importance of fully qualifying the >catalog query used to discover tables to process. Regarding the search_path business, there is actually similar business in expand_table_name_patterns() for pg_dump if you look close by, so as users calling --table don't have to specify the schema, and just stick with patterns. + /* +* Prepare the list of tables to process by querying the catalogs. +* +* Since we execute the constructed query with the default search_path +* (which could be unsafe), it is important to fully qualify everything. +*/ It is not only important, but also absolutely mandatory, so let's make the comment insisting harder on that point. From this point of view, the query that you are building is visibly correct. + appendStringLiteralConn(_query, just_table, conn); + appendPQExpBuffer(_query, "::pg_catalog.regclass\n"); + + pg_free(just_table); + + cell = cell->next; + if (cell == NULL) + appendPQExpBuffer(_query, " )\n"); Hm. It seems to me that you are duplicating what processSQLNamePattern() does, so we ought to use it. And it is possible to control the generation of the different WHERE clauses with a single query based on the number of elements in the list. Perhaps I am missing something? It looks unnecessary to export split_table_columns_spec() as well. - qr/statement: ANALYZE;/, + qr/statement: ANALYZE pg_catalog\./, Or we could just use "ANALYZE \.;/", perhaps patching it first. Perhaps my regexp here is incorrect, but I just mean to check for an ANALYZE query which begins by "ANALYZE " and finishes with ";", without caring about what is in-between. This would make the tests more portable. > - 0003 includes additional documentation changes explaining the main >uses of --min-xid-age and --min-mxid-age and linking to the >existing wraparound documentation. +$node->issues_sql_like( + [ 'vacuumdb', '--table=pg_class', '--min-mxid-age=123456789', 'postgres'], + qr/AND GREATEST\(pg_catalog.mxid_age\(c.relminmxid\), pg_catalog.mxid_age\(t.relminmxid\)\) OPERATOR\(pg_catalog.>=\) 123456789/, + 'vacuumdb --table --min-mxid-age'); +$node->issues_sql_like( + [ 'vacuumdb', '--min-xid-age=987654321', 'postgres' ], + qr/AND GREATEST\(pg_catalog.age\(c.relfrozenxid\), pg_catalog.age\(t.relfrozenxid\)\) OPERATOR\(pg_catalog.>=\) 987654321/, + 'vacuumdb --min-xid-age'); It may be better to use numbers which make sure that no relations are actually fetched, so as if the surrounding tests are messed up we never make them longer than necessary just to check the shape of the query. -- Michael signature.asc Description: PGP signature
RE: speeding up planning with partitions
Imai-san,
If the time for EXECUTE differs cleary before and after the creation of the
generic plan, why don't you try to count the function calls for each EXECUTE?
Although I haven't tried it, but you can probably do it with SystemTap, like
this:
probe process("your_postgres_path").function("*").call {
/* accumulate the call count in an associative array */
}
Then, sort the functions by their call counts. You may find some notable
difference between the 5th and 7th EXECUTE.
Regards
Takayuki Tsunakawa
Re: Typo: llvm*.cpp files identified as llvm*.c
On Tue, Jan 22, 2019 at 01:43:32PM +0900, Amit Langote wrote: > Hi, > > Attached find a patch to fix $subject. It is like that since 31bc604. Andres, would you take care of it? It is your commit after all.. -- Michael signature.asc Description: PGP signature
Re: pgsql: Restrict the use of temporary namespace in two-phase transaction
On Tue, Jan 22, 2019 at 01:47:05PM +0900, Masahiko Sawada wrote: > Or can we make the test script set force_parallel_mode = off? Since > the failure case is a very rare in real world I think that it might be > better to change the test scripts rather than changing properly of > current_schema(). Please see 396676b, which is in my opinion a quick workaround to the problem. Even if that's a rare case, it would be confusing to the user to see it :( -- Michael signature.asc Description: PGP signature
Re: Early WIP/PoC for inlining CTEs
On 22/01/2019 02:40, Andreas Karlsson wrote: On 1/18/19 9:34 PM, Robert Haas wrote: On Thu, Jan 17, 2019 at 10:48 AM Andreas Karlsson wrote: On 1/11/19 8:10 PM, Robert Haas wrote: WITH cte_name [[NOT] MATERIALIZED] AS (query) main_query... Hm, when would one want "NOT MATERIALIZED"? I am not sure I see the usefulness of forcing inlining other than if we by default do not inline when a CTE is referenced multiple times. When the planner materializes it, but the performance of the resulting plan therefore sucks, I suppose. I don't feel super-strongly about this, and Tom is right that there may be cases where materialization is just not practical due to implementation restrictions. But it's not crazy to imagine that inlining a multiply-referenced CTE might create opportunities for optimization at each of those places, perhaps not the same ones in each case, whereas materializing it results in doing extra work. I see. I have a minor biksheddish question about the syntax. You proposed: WITH cte_name [[NOT] MATERIALIZED] AS (query) main_query While Andrew proposed: WITH cte_name AS [[NOT] MATERIALIZED] (query) main_query Do people have any preference between these two? Andreas +1 For putting the 'AS' earlier, 2nd option, I think it reads better. Cheers, Gavin
Re: pgsql: Restrict the use of temporary namespace in two-phase transaction
On Sat, Jan 19, 2019 at 5:05 AM Robert Haas wrote: > > On Thu, Jan 17, 2019 at 8:08 PM Tom Lane wrote: > > > Anyway, it seems to me that this is pointing out to another issue: > > > current_schema() can trigger a namespace creation, hence shouldn't we > > > mark it as PARALLEL UNSAFE and make sure that we never run into this > > > problem? > > > > That seems a bit annoying, but maybe we have little choice? > > The only other option I see is to make current_schema() not trigger a > namespace creation. > Or can we make the test script set force_parallel_mode = off? Since the failure case is a very rare in real world I think that it might be better to change the test scripts rather than changing properly of current_schema(). Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Typo: llvm*.cpp files identified as llvm*.c
Hi, Attached find a patch to fix $subject. Thanks, Amit diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp index ba3907c452..9c6e8026e7 100644 --- a/src/backend/jit/llvm/llvmjit_error.cpp +++ b/src/backend/jit/llvm/llvmjit_error.cpp @@ -9,7 +9,7 @@ * Copyright (c) 2016-2019, PostgreSQL Global Development Group * * IDENTIFICATION - * src/backend/jit/llvm/llvmjit_error.c + * src/backend/jit/llvm/llvmjit_error.cpp * *- */ diff --git a/src/backend/jit/llvm/llvmjit_inline.cpp b/src/backend/jit/llvm/llvmjit_inline.cpp index 49e5ee8954..96fc68a356 100644 --- a/src/backend/jit/llvm/llvmjit_inline.cpp +++ b/src/backend/jit/llvm/llvmjit_inline.cpp @@ -14,7 +14,7 @@ * Copyright (c) 2016-2019, PostgreSQL Global Development Group * * IDENTIFICATION - * src/backend/lib/llvmjit/llvmjit_inline.c + * src/backend/lib/llvmjit/llvmjit_inline.cpp * *- */ diff --git a/src/backend/jit/llvm/llvmjit_wrap.cpp b/src/backend/jit/llvm/llvmjit_wrap.cpp index 43c9314fe0..18876ec520 100644 --- a/src/backend/jit/llvm/llvmjit_wrap.cpp +++ b/src/backend/jit/llvm/llvmjit_wrap.cpp @@ -6,7 +6,7 @@ * Copyright (c) 2016-2019, PostgreSQL Global Development Group * * IDENTIFICATION - * src/backend/lib/llvm/llvmjit_wrap.c + * src/backend/lib/llvm/llvmjit_wrap.cpp * *- */
Re: Logical decoding for operations on zheap tables
On Sat, Jan 12, 2019 at 5:02 PM Amit Kapila wrote: > > Fair enough. I think that for now (and maybe for the first version > that can be committed) we might want to use heap tuple format. There > will be some overhead but I think code-wise, things will be simpler. > I have prototyped it for Insert and Delete operations of zheap and the > only thing that is required are new decode functions, see the attached > patch. I have done very minimal testing of this patch as this is just > to show you and others the direction we are taking (w.r.t tuple > format) to support logical decoding in zheap. + */ + zhtup = DecodeXLogZTuple(tupledata, datalen); + reloid = RelidByRelfilenode(change->data.tp.relnode.spcNode, + change->data.tp.relnode.relNode); + relation = RelationIdGetRelation(reloid); We need to start a transaction for fetching the relation if it's a walsender process. I have fixed this issue in the patch and also implemented decode functions for zheap update and multi-insert. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com decode_zops_v4.patch Description: Binary data
Re: problems with foreign keys on partitioned tables
On 2019/01/22 8:30, Alvaro Herrera wrote:
> Hi Amit,
>
> Will you please rebase 0002? Please add your proposed tests cases to
> it, too.
Done. See the attached patches for HEAD and PG 11.
Thanks,
Amit
From 432c4551990d0da1c77b6b9523296b0a2a0a5119 Mon Sep 17 00:00:00 2001
From: amit
Date: Tue, 22 Jan 2019 13:20:54 +0900
Subject: [PATCH] Do not track foreign key inheritance by dependencies
Inheritance information maintained in pg_constraint is enough to
prevent a child constraint to be dropped and for them to be dropped
when the parent constraint is dropped. So, do not create
dependencies between the parent foreign key constraint and its
children.
Also, fix ATAddForeignKeyConstraint to set connoniherit on the parent
constraint correctly.
---
src/backend/catalog/pg_constraint.c | 27 +--
src/backend/commands/indexcmds.c | 22 --
src/backend/commands/tablecmds.c | 24
src/test/regress/expected/foreign_key.out | 17 +++--
src/test/regress/sql/foreign_key.sql | 14 +-
5 files changed, 73 insertions(+), 31 deletions(-)
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 446b54b9ff..855d57c65a 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -762,8 +762,8 @@ AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
* ConstraintSetParentConstraint
* Set a partition's constraint as child of its parent table's
*
- * This updates the constraint's pg_constraint row to show it as inherited, and
- * add a dependency to the parent so that it cannot be removed on its own.
+ * This updates the constraint's pg_constraint row to change its inheritance
+ * properties.
*/
void
ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
@@ -772,8 +772,6 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
Form_pg_constraint constrForm;
HeapTuple tuple,
newtup;
- ObjectAddress depender;
- ObjectAddress referenced;
constrRel = table_open(ConstraintRelationId, RowExclusiveLock);
tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(childConstrId));
@@ -785,25 +783,26 @@ ConstraintSetParentConstraint(Oid childConstrId, Oid
parentConstrId)
{
constrForm->conislocal = false;
constrForm->coninhcount++;
+
+ /*
+* An inherited foreign key constraint can never have more than
one
+* parent, because inheriting foreign keys is only allowed for
+* partitioning where multiple inheritance is disallowed.
+*/
+ Assert(constrForm->coninhcount == 1);
+
constrForm->conparentid = parentConstrId;
CatalogTupleUpdate(constrRel, >t_self, newtup);
-
- ObjectAddressSet(referenced, ConstraintRelationId,
parentConstrId);
- ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
-
- recordDependencyOn(, ,
DEPENDENCY_INTERNAL_AUTO);
}
else
{
constrForm->coninhcount--;
- if (constrForm->coninhcount <= 0)
- constrForm->conislocal = true;
+ /* See the above comment. */
+ Assert(constrForm->coninhcount == 0);
+ constrForm->conislocal = true;
constrForm->conparentid = InvalidOid;
- deleteDependencyRecordsForClass(ConstraintRelationId,
childConstrId,
-
ConstraintRelationId,
-
DEPENDENCY_INTERNAL_AUTO);
CatalogTupleUpdate(constrRel, >t_self, newtup);
}
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3edc94308e..6c8aa5d149 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -972,8 +972,26 @@ DefineIndex(Oid relationId,
/* Attach index to parent and
we're done. */
IndexSetParentIndex(cldidx,
indexRelationId);
if (createdConstraintId !=
InvalidOid)
-
ConstraintSetParentConstraint(cldConstrOid,
-
createdConstraintId);
+ {
+ ObjectAddress depender;
+ ObjectAddress
referenced;
+
+
Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Thanks for making those changes.
On Fri, 18 Jan 2019 at 10:03, Tomas Vondra wrote:
> A couple of items remains:
>
> > 15. I see you broke out the remainder of the code from
> > clauselist_selectivity() into clauselist_selectivity_simple(). The
> > comment looks like just a copy and paste from the original. That
> > seems like quite a bit of duplication. Is it better to maybe trim down
> > the original one?
>
> I don't follow - where do you see the code duplication? Essentially, we
> have clauselist_selectivity and clauselist_selectivity_simple, but the
> first one calls the second one. The "simple" version is needed because
> in some cases we need to perform estimation without multivariate stats
> (e.g. to prevent infinite loop due to recursion).
It was the comment duplication that I was complaining about. I think
clauselist_selectivity()'s comment can be simplified to mention it
attempts to apply extended statistics and applies
clauselist_selectivity_simple on any stats that remain. Plus any
details that are specific to extended statistics. That way if anyone
wants further detail on what happens to the remaining clauses they can
look at the comment above clauselist_selectivity_simple().
> > 18. In dependencies_clauselist_selectivity() there seem to be a new
> > bug introduced. We do:
> >
> > /* mark this one as done, so we don't touch it again. */
> > *estimatedclauses = bms_add_member(*estimatedclauses, listidx);
> >
> > but the bms_is_member() check that skipped these has been removed.
> >
> > It might be easier to document if we just always do:
> >
> > if (bms_is_member(listidx, *estimatedclauses))
> > continue;
> >
> > at the start of both loops. list_attnums can just be left unset for
> > the originally already estimatedclauses.
>
> This was already discussed - I don't think there's any bug, but I'll
> look into refactoring the code somehow to make it clear.
On looking at this a bit more it seems that since the estimated attr
is removed from the clauses_attnums Bitmapset that
find_strongest_dependency() will no longer find a dependency for that
clause and dependency_implies_attribute() will just return false where
the bms_is_member(listidx, *estimatedclauses) would have done
previously. I'll mean we could get more calls of
dependency_implies_attribute(), but that function is even cheaper than
bms_is_member() so I guess there's no harm in this change.
> > 25. Does statext_is_compatible_clause_internal)_ need to skip over
> > RelabelTypes?
>
> I don't think it should, because while RelabelType nodes represent casts
> to binary-compatible types, there's no guarantee the semantics actually
> is compatible.
The code that looks through RelabelTypes for normal stats is in
examine_variable(). This code allows the following to estimate 4 rows.
I guess if we didn't use that then we'd just need to treat it like
some unknown expression and use DEFAULT_NUM_DISTINCT.
create table a (t varchar);
insert into a select v.v from (values('One'),('Two'),('Three')) as
v(v), generate_Series(1,4);
analyze a;
explain (summary off, timing off, analyze) select * from a where t = 'One';
QUERY PLAN
-
Seq Scan on a (cost=0.00..1.15 rows=4 width=4) (actual rows=4 loops=1)
Filter: ((t)::text = 'One'::text)
Rows Removed by Filter: 8
(3 rows)
Why do you think its okay for the normal stats to look through
RelabelTypes but not the new code you're adding?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: Control your disk usage in PG: Introduction to Disk Quota Extension
> > > For this particular purpose, I don't immediately see why you need a > > hook in both places. If ReadBuffer is called with P_NEW, aren't we > > guaranteed to end up in smgrextend()? > Yes, that's a bit awkward. Hi Michael, we revisit the ReadBuffer hook and remove it in the latest patch. ReadBuffer hook is original used to do enforcement(e.g. out of diskquota limit) when query is loading data. We plan to put the enforcement work of running query to separate diskquota worker process. Let worker process to detect the backends to be cancelled and send SIGINT to these backends. So there is no need for ReadBuffer hook anymore. Our patch currently only contains smgr related hooks to catch the file change and get the Active Table list for diskquota extension. Thanks Hubert. On Mon, Jan 7, 2019 at 6:56 PM Haozhou Wang wrote: > Thanks very much for your comments. > > To the best of my knowledge, smgr is a layer that abstract the storage > operations. Therefore, it is a good place to control or collect information > the storage operations without touching the physical storage layer. > Moreover, smgr is coming with actual disk IO operation (not consider the > OS cache) for postgres. So we do not need to worry about the buffer > management in postgres. > It will make the purpose of hook is pure: a hook for actual disk IO. > > Regards, > Haozhou > > On Wed, Dec 26, 2018 at 1:56 PM Michael Paquier > wrote: > >> On Wed, Nov 21, 2018 at 09:47:44AM -0500, Robert Haas wrote: >> > +1 for adding some hooks to support this kind of thing, but I think >> > the names you've chosen are not very good. The hook name should >> > describe the place from which it is called, not the purpose for which >> > one imagines that it will be used, because somebody else might imagine >> > another use. Both BufferExtendCheckPerms_hook_type and >> > SmgrStat_hook_type are imagining that they know what the hook does - >> > CheckPerms in the first case and Stat in the second case. >> >> I personally don't mind making Postgres more pluggable, but I don't >> think that we actually need the extra ones proposed here at the layer >> of smgr, as smgr is already a layer designed to call an underlying set >> of APIs able to extend, unlink, etc. depending on the storage type. >> >> > For this particular purpose, I don't immediately see why you need a >> > hook in both places. If ReadBuffer is called with P_NEW, aren't we >> > guaranteed to end up in smgrextend()? >> >> Yes, that's a bit awkward. >> -- >> Michael > > -- Thanks Hubert Zhang disk_quota_hooks_v3.patch Description: Binary data
Re: Early WIP/PoC for inlining CTEs
On 1/10/19 2:28 AM, Andreas Karlsson wrote: Here is a new version of the patch with added tests, improved comments, some minor code cleanup and most importantly slightly changed logic for when we should inline. Add ctematerialized to the JumbleExpr() in pg_stat_statements on suggestion from Andrew Gierth. I think that is the correct thing to do since it can have a major impact on performance. Andreas diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index f177ebaa2c..6d456f6bce 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2925,6 +2925,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node) /* we store the string name because RTE_CTE RTEs need it */ APP_JUMB_STRING(cte->ctename); +APP_JUMB(cte->ctematerialized); JumbleQuery(jstate, castNode(Query, cte->ctequery)); } break; diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index bb92d9d37a..8c26dd1f26 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -1888,7 +1888,7 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t -- join in CTE EXPLAIN (VERBOSE, COSTS OFF) -WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; +WITH t (c1_1, c1_3, c2_1) AS MATERIALIZED (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; QUERY PLAN - Limit @@ -1905,7 +1905,7 @@ WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 Output: t.c1_1, t.c2_1, t.c1_3 (12 rows) -WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; +WITH t (c1_1, c1_3, c2_1) AS MATERIALIZED (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; c1_1 | c2_1 --+-- 101 | 101 diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index f438165650..56602a164c 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -493,8 +493,8 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR SHARE; -- join in CTE EXPLAIN (VERBOSE, COSTS OFF) -WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; -WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; +WITH t (c1_1, c1_3, c2_1) AS MATERIALIZED (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; +WITH t (c1_1, c1_3, c2_1) AS MATERIALIZED (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10; -- ctid with whole-row reference EXPLAIN (VERBOSE, COSTS OFF) SELECT t1.ctid, t1, t2, t1.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10; diff --git a/doc/src/sgml/queries.sgml b/doc/src/sgml/queries.sgml index 88bc189646..92f180c5cf 100644 --- a/doc/src/sgml/queries.sgml +++ b/doc/src/sgml/queries.sgml @@ -2199,6 +2199,8 @@ SELECT n FROM t LIMIT 100; + TODO: Update this for inlining and MATERIALIZED. + A useful property of WITH queries is that they are evaluated only once per execution of the parent query, even if they are referred to more than once by the parent query or sibling WITH queries. diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 4db8142afa..92ede4fc6c 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -72,7 +72,7 @@ SELECT [ ALL | DISTINCT [ ON ( expressionand with_query is: -with_query_name [ ( column_name [, ...] ) ] AS ( select | values | insert | update | delete ) +with_query_name [ ( column_name [, ...] ) ] AS [ MATERIALIZED ] ( select | values | insert | update | delete ) TABLE [ ONLY ] table_name [ * ] @@ -273,6 +273,8 @@ TABLE [ ONLY ] table_name [ * ] +TODO: Update this
Re: Allowing extensions to find out the OIDs of their member objects
On 01/21/19 21:45, Tom Lane wrote: > are concerned, but for cross-extension references it seems a > lot better to be looking for "postgis / function_foo_int_int" > than for "postgis / 3". I wonder if postgis might offer a .h file with FUNCTION_POSTGIS_FOO_INT_INT defined as 3, which extensions intending to use foo could be built against. Any that aren't could still search by name and signature. In the case of calls from core to some pluggable extension, of course the .h file would be in core, with the implementing extensions expected to build against it: in thy extension shalt thou provide XMLCAST at index 1, XMLITERATE at index 2, etc. Regards, -Chap
Re: Record last password change
On Wed, Dec 12, 2018 at 07:30:18AM -0700, Bear Giles wrote: > BTW another solution is SSO, e.g., Kerberos. I still need to submit a patch to > pgsql to handle it better(*) but with postgresql itself you sign into the > system and then the database server will just know who you are. You don't have > to worry about remembering a new password for postgresql. X.509 (digital > certs) > are another possibility and I know you can tie them to a smart card but again > I > don't know how well we could integrate it into pgsql. (Good to talk to you again.) I recently wrote a blog entry about putting the certificate and its private key on removable media: https://momjian.us/main/blogs/pgblog/2019.html#January_16_2019 and mentioned the value of PIV over removable media: https://momjian.us/main/blogs/pgblog/2019.html#January_14_2019 I can't think of a way to access a smart card for authentication, though I did wrote a presentation on how to use PIV devices for server-side and client-side encryption: https://momjian.us/main/writings/crypto_hw_use.pdf -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Record last password change
On Tue, Dec 11, 2018 at 09:56:54AM -0500, Tom Lane wrote: > Michael Banck writes: > > The same was requested in https://dba.stackexchange.com/questions/91252/ > > how-to-know-when-postgresql-password-is-changed so I was wondering > > whether this would be a welcome change/addition, or whether people think > > it's not worth bothering to implement it? > > This has all the same practical problems as recording object creation > times, which we're not going to do either. (You can consult the > archives for details, but from memory, the stickiest aspects revolve > around what to do during dump/reload. Although even CREATE OR REPLACE > offers interesting definitional questions. In the end there are just > too many different behaviors that somebody might want.) I wrote a blog on this topic in 2017: https://momjian.us/main/blogs/pgblog/2017.html#November_21_2017 -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Thread-unsafe coding in ecpg
On 1/21/19 3:25 PM, Tom Lane wrote: > "Joshua D. Drake" writes: >> On 1/21/19 12:05 PM, Tom Lane wrote: >>> Is there a newer version of mingw that does have this functionality? >> Apparently this can be done with thee 64bit version: >> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw > Hmm, the followup question makes it sound like it still didn't work :-(. > > However, since the mingw build is autoconf-based, seems like we can > install a configure check instead of guessing. Will make it so. > > Task for somebody else: run a MinGW64 buildfarm member. > > I could set that up in just about two shakes of a lamb's tail - I have a script to do so all tested on vagrant/aws within the last few months. What I don't have is resources. My Windows resources are pretty much tapped out. I would need either some modest hardware or a Windows VM somewhere in the cloud - could be anywhere but I'm most at home on AWS. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allowing extensions to find out the OIDs of their member objects
Chapman Flack writes: > On 01/21/19 18:52, Tom Lane wrote: >> I'm probably going to push forward with the OID mapping idea instead. > As it happens, I'd been recently thinking[1] about a way that certain > SQL/XML functionality could be provided by a pluggable selection of > different extensions. > And I think a use case like that could be rather elegantly served by > the OID mapping idea, more so than by a fixed-OID-range-per-extension > approach. Hm, yeah. One use-case that's been in the back of my mind is cross-extension references; for example, what if PostGIS wants to map a call to one of its own functions into an indexable operator that's defined by the btree_gist extension? What you're talking about, IIUC, is a similar kind of reference only it goes from the core code to an extension. This line of thought says that the identifiers exposed by what I was calling a SET MAP command would soon become part of the de facto API of an extension: you'd not want to change them for fear that some other extension was relying on them. Perhaps this also gives some impetus to the lets-use-identifiers- not-numbers approach that Andrew was pushing. I didn't care for that too much so far as an extension's own internal references are concerned, but for cross-extension references it seems a lot better to be looking for "postgis / function_foo_int_int" than for "postgis / 3". On the third hand you could also say that such references should just use name-based lookup and not a facility that's designed to bypass the expense of that. Loading additional functionality onto said facility just reduces its desired speed advantage. (That is, in the terms of what I think you mean for the SQL/XML business, an extension that's meant to serve that purpose would be required to provide functions with specified names and signatures, and the core would look them up that way rather than with any behind-the-scenes API.) regards, tom lane
Re: Pluggable Storage - Andres's take
On Tue, Jan 22, 2019 at 12:15 PM Andres Freund wrote:
> Hi,
>
> Thanks!
>
> On 2019-01-22 11:51:57 +1100, Haribabu Kommi wrote:
> > Attached the patch for removal of scan_update_snapshot
> > and also the rebased patch of reduction in use of t_tableOid.
>
> I'll soon look at the latter.
>
Thanks.
>
> > > - consider removing table_gimmegimmeslot()
> > > - add substantial docs for every callback
> > >
> >
> > Will work on the above two.
>
> I think it's easier if I do the first, because I can just do it while
> rebasing, reducing unnecessary conflicts.
>
>
OK. I will work on the doc changes.
> > > While I saw an initial attempt at writing smgl docs for the table AM
> > > API, I'm not convinced that's the best approach. I think it might make
> > > more sense to have high-level docs in sgml, but then do all the
> > > per-callback docs in tableam.h.
> > >
> >
> > OK, I will update the sgml docs accordingly.
> > Index AM has per callback docs in the sgml, refactor them also?
>
> I don't think it's a good idea to tackle the index docs at the same time
> - this patchset is already humongously large...
>
OK.
>
> > diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> > index 62c5f9fa9f..3dc1444739 100644
> > --- a/src/backend/access/heap/heapam_handler.c
> > +++ b/src/backend/access/heap/heapam_handler.c
> > @@ -2308,7 +2308,6 @@ static const TableAmRoutine heapam_methods = {
> > .scan_begin = heap_beginscan,
> > .scan_end = heap_endscan,
> > .scan_rescan = heap_rescan,
> > - .scan_update_snapshot = heap_update_snapshot,
> > .scan_getnextslot = heap_getnextslot,
> >
> > .parallelscan_estimate = table_block_parallelscan_estimate,
> > diff --git a/src/backend/executor/nodeBitmapHeapscan.c
> b/src/backend/executor/nodeBitmapHeapscan.c
> > index 59061c746b..b48ab5036c 100644
> > --- a/src/backend/executor/nodeBitmapHeapscan.c
> > +++ b/src/backend/executor/nodeBitmapHeapscan.c
> > @@ -954,5 +954,9 @@ ExecBitmapHeapInitializeWorker(BitmapHeapScanState
> *node,
> > node->pstate = pstate;
> >
> > snapshot = RestoreSnapshot(pstate->phs_snapshot_data);
> > - table_scan_update_snapshot(node->ss.ss_currentScanDesc, snapshot);
> > + Assert(IsMVCCSnapshot(snapshot));
> > +
> > + RegisterSnapshot(snapshot);
> > + node->ss.ss_currentScanDesc->rs_snapshot = snapshot;
> > + node->ss.ss_currentScanDesc->rs_temp_snap = true;
> > }
>
> I was rather thinking that we'd just move this logic into
> table_scan_update_snapshot(), without it invoking a callback.
>
OK. Changed accordingly.
But the table_scan_update_snapshot() function is moved into tableam.c,
to avoid additional header file snapmgr.h inclusion in tableam.h
Regards,
Haribabu Kommi
Fujitsu Australia
0002-Removal-of-scan_update_snapshot-callback.patch
Description: Binary data
RE: speeding up planning with partitions
I measured the latency of queries executed before and after creating generic plan with master + v15-patch. In this test, table is partitioned into 4k partitions. I executed 400,0001 queries by pgbench. I changed the timing of creating generic plan at 1st, 10,001st, 20,001st, 50,001st, ..., 390,001st by changing the source code. I run the test with setting both plan_cache_mode = auto and plan_cache_mode = force_custom_plan. The results is below. The value in before columns is showing the average latency of queries before creating generic plan in microsec. The value in after columns is showing the average latency of queries after creating generic plan in microsec. [auto] time of creating generic plan | before[usec] | after[usec] 1st 531 142 10,001st 144 141 20,001st 141 144 50,001st 133 140 200,001st 131 143 390,001st 130 138 [force_custom_plan] time of creating generic plan | before[usec] | after[usec] 1st 10,001st 144 129 20,001st 137 131 50,001st 133 134 200,001st 131 133 390,001st 132 131 * A generic plan is actually not created with plan_cache_mode = force_custom_plan. Looking at the results of force_custom_plan, the latency of first 10,000 transactions is 144 microsec, and the latency of first 50,000 transactions is 133 microsec. I think that is because in the early transactions, relcache hash table is not hot (as David mentioned?). Comparing the latencies in after column between auto and force_custom_plan, auto ones are higher about 8% than force_custom_plan ones. That is, it seems creating generic plan affects the latency of queries executed after creating generic plan. On Mon, Jan 21, 2019 at 1:32 AM, David Rowley wrote: > It would be interesting to see the profiles of having the generic plan > being built on the 6th execution vs the 400,000th execution. > > I'd thought maybe one difference would be the relcache hash table > having been expanded greatly after the generic plan was created Does it mean that in the executing queries after the generic plan was created, the time of searching entry in the relcache hash table becomes slow and it increases the latency? > but I > see even the generic plan is selecting a random partition, so the > cache would have ended up with that many items eventually anyway, and > since we're talking in the odds of 7.8k TPS with 4k partitions, it > would have only have taken about 2-3 seconds out of the 60 second run > to hit most or all of those partitions anyway. And does it mean even if we executes a lot of custom plan without creating generic plan, cache would have been ended up to the same size of which is after creating generic plan? Anyway, I'll check the relcache size. Since I don't know how to get profile at the just time of building generic plan, I'll use MemoryContextStats(MemoryContext*) function to check the relcache size at before/after building generic plan and at after executing a lot of custom plans. Yoshikazu Imai
Re: Allowing extensions to find out the OIDs of their member objects
On 01/21/19 18:52, Tom Lane wrote: > I'm probably going to push forward with the OID mapping idea instead. As it happens, I'd been recently thinking[1] about a way that certain SQL/XML functionality could be provided by a pluggable selection of different extensions. And I think a use case like that could be rather elegantly served by the OID mapping idea, more so than by a fixed-OID-range-per-extension approach. So +1. -Chap [1] https://wiki.postgresql.org/wiki/PostgreSQL_vs_SQL/XML_Standards#Proposal_2:_desugaring_to_calls_on_normal_extension_functions
Re: Allowing extensions to find out the OIDs of their member objects
Andrew Gierth writes: > "Tom" == Tom Lane writes: > Tom> I thought about extending the extension infrastructure to provide > Tom> some way of retrieving relevant OIDs. We could imagine, for > Tom> instance, that an extension script has a way to say "this function > Tom> is object number three within this extension", ... > I suggest using string tags rather than object numbers: Meh ... that increases the cost of individual lookups substantially. The implementation I had in mind was that an extension would do a once-per-session catalog lookup to fetch an array of OIDs [1], and then individual operations would just index into that array. If what we provide is string tags, the cost per lookup goes up by two or three orders of magnitude, for benefits that seem pretty hypothetical. The in-catalog representation gets a lot more complex too, as it can't just be "oid[]". (No, I do not wish to hear the word JSON here.) I don't buy any of your suggested benefits: > 1. easier to read and maintain The SQL-level API that I'm imagining would look roughly like a command like this at the end of an extension's script: ALTER EXTENSION extname SET MAP OBJECT 1 IS FUNCTION foo(int, int), OBJECT 2 IS OPERATOR +(float, float), ... where the parts after "IS" should be able to use the same production as for "member_object" in ALTER EXTENSION ADD/DROP. Execution of this would replace an oid[] field in the extension's pg_extension row. So yeah, we could replace the numbers by identifiers in this command, but does that really buy us much maintainability? Any intelligent extension author is going to have a C header with something like #define MY_FUNCTION_FOO_INT_INT 1 #define MY_OPERATOR_PLUS_FLOAT_FLOAT 2 which she has to keep in sync with the ALTER SET MAP in her extension script. Using names in the SET MAP just changes the contents of those #defines to strings, which isn't moving the goalposts much for maintainability. > 2. an object might be given many tags, some of them automatically > 3. it might make sense to provide a function that the extension can use > to ask "is oid X one of my objects with tag 'foo'" (e.g. to match one of > a set of related functions) in addition to looking up specific oids by > tag I'm not seeing that either of these have actual real-world use cases, at least not use-cases with the same constraints that the OID mapping problem has. In particular, we're going to have to lock down use of the SET MAP command pretty hard, since the ability to insert a different object than a support function thought it was calling would be an easy security hole. That seems like it lets out many of the potential applications of the sort of object labeling you're talking about. (I'd also wonder why such labeling would be needed only for objects in extensions.) regards, tom lane [1] The possibility of needing to flush that cache complicates this, but it'd complicate the other thing too.
Re: Feature: temporary materialized views
On 1/21/19 3:31 AM, Andreas Karlsson wrote:
Here is a a stab at refactoring this so the object creation does not
happen in a callback.
Rebased my patch on top of Andres's pluggable storage patches. Plus some
minor style changes.
Andreas
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 2bc8f928ea..7ef3794e08 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -55,16 +55,15 @@ typedef struct
{
DestReceiver pub; /* publicly-known function pointers */
IntoClause *into; /* target relation specification */
+ ObjectAddress reladdr; /* address of rel, for intorel_startup */
/* These fields are filled by intorel_startup: */
Relation rel; /* relation to write to */
- ObjectAddress reladdr; /* address of rel, for ExecCreateTableAs */
CommandId output_cid; /* cmin to insert in output tuples */
int hi_options; /* heap_insert performance options */
BulkInsertState bistate; /* bulk insert state */
} DR_intorel;
-/* utility functions for CTAS definition creation */
-static ObjectAddress create_ctas_internal(List *attrList, IntoClause *into);
+/* utility function for CTAS definition creation */
static ObjectAddress create_ctas_nodata(List *tlist, IntoClause *into);
/* DestReceiver routines for collecting data */
@@ -75,16 +74,18 @@ static void intorel_destroy(DestReceiver *self);
/*
- * create_ctas_internal
+ * create_ctas_nodata
*
- * Internal utility used for the creation of the definition of a relation
- * created via CREATE TABLE AS or a materialized view. Caller needs to
- * provide a list of attributes (ColumnDef nodes).
+ * Create CTAS or materialized view without the data, starting from the
+ * targetlist of the SELECT or view definition.
*/
static ObjectAddress
-create_ctas_internal(List *attrList, IntoClause *into)
+create_ctas_nodata(List *tlist, IntoClause *into)
{
- CreateStmt *create = makeNode(CreateStmt);
+ List *attrList;
+ ListCell *t,
+ *lc;
+ CreateStmt *create;
bool is_matview;
char relkind;
Datum toast_options;
@@ -96,71 +97,6 @@ create_ctas_internal(List *attrList, IntoClause *into)
relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION;
/*
- * Create the target relation by faking up a CREATE TABLE parsetree and
- * passing it to DefineRelation.
- */
- create->relation = into->rel;
- create->tableElts = attrList;
- create->inhRelations = NIL;
- create->ofTypename = NULL;
- create->constraints = NIL;
- create->options = into->options;
- create->oncommit = into->onCommit;
- create->tablespacename = into->tableSpaceName;
- create->if_not_exists = false;
-
- /*
- * Create the relation. (This will error out if there's an existing view,
- * so we don't need more code to complain if "replace" is false.)
- */
- intoRelationAddr = DefineRelation(create, relkind, InvalidOid, NULL, NULL);
-
- /*
- * If necessary, create a TOAST table for the target table. Note that
- * NewRelationCreateToastTable ends with CommandCounterIncrement(), so
- * that the TOAST table will be visible for insertion.
- */
- CommandCounterIncrement();
-
- /* parse and validate reloptions for the toast table */
- toast_options = transformRelOptions((Datum) 0,
- create->options,
- "toast",
- validnsps,
- true, false);
-
- (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
-
- NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
-
- /* Create the "view" part of a materialized view. */
- if (is_matview)
- {
- /* StoreViewQuery scribbles on tree, so make a copy */
- Query *query = (Query *) copyObject(into->viewQuery);
-
- StoreViewQuery(intoRelationAddr.objectId, query, false);
- CommandCounterIncrement();
- }
-
- return intoRelationAddr;
-}
-
-
-/*
- * create_ctas_nodata
- *
- * Create CTAS or materialized view when WITH NO DATA is used, starting from
- * the targetlist of the SELECT or view definition.
- */
-static ObjectAddress
-create_ctas_nodata(List *tlist, IntoClause *into)
-{
- List *attrList;
- ListCell *t,
- *lc;
-
- /*
* Build list of ColumnDefs from non-junk elements of the tlist. If a
* column name list was specified in CREATE TABLE AS, override the column
* names in the query. (Too few column names are OK, too many are not.)
@@ -213,8 +149,56 @@ create_ctas_nodata(List *tlist, IntoClause *into)
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("too many column names were specified")));
- /* Create the relation definition using the ColumnDef list */
- return create_ctas_internal(attrList, into);
+ /*
+ * Create the target relation by faking up a CREATE TABLE parsetree and
+ * passing it to DefineRelation.
+ */
+ create = makeNode(CreateStmt);
+ create->relation = into->rel;
+ create->tableElts = attrList;
+ create->inhRelations = NIL;
+ create->ofTypename = NULL;
+ create->constraints = NIL;
+ create->options = into->options;
+ create->oncommit = into->onCommit;
+
Re: explain plans with information about (modified) gucs
On 1/22/19 1:35 AM, John Naylor wrote: > On Sun, Jan 20, 2019 at 2:31 PM Tomas Vondra > wrote: >> Attached is v6 of the patch, adopting "settings" instead of "guc". > > Ok, looks good. I tried changing config values with the .conf file, > alter system, and alter database, and tried a few queries with > auto_explain. I made a pass through the config parameters, and don't > see anything obviously left out. I have no comments on the source > code. > > One thing stands out: For the docs on auto_explain, all other options > have "Only superusers can change this setting.", but log_settings > doesn't. Yes, that's an omission in the docs. Will fix. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allowing extensions to find out the OIDs of their member objects
> "Tom" == Tom Lane writes: Tom> I thought about extending the extension infrastructure to provide Tom> some way of retrieving relevant OIDs. We could imagine, for Tom> instance, that an extension script has a way to say "this function Tom> is object number three within this extension", and while running Tom> the script we make a catalog entry showing that object number Tom> three has OID thus-and-so, and then that catalog entry can be Tom> consulted to get the right OID (by C code that has hard-wired Tom> knowledge that object number three is the function it cares Tom> about). This is still kind of messy, because aside from the Tom> hand-assigned object numbers you'd have to use the extension name Tom> as part of the lookup key, making the name into something the C Tom> code critically depends on. We don't have ALTER EXTENSION RENAME, Tom> so maybe that's okay, but it seems painful to say that we can Tom> never have it. I suggest using string tags rather than object numbers: 1. easier to read and maintain 2. an object might be given many tags, some of them automatically 3. it might make sense to provide a function that the extension can use to ask "is oid X one of my objects with tag 'foo'" (e.g. to match one of a set of related functions) in addition to looking up specific oids by tag -- Andrew (irc:RhodiumToad)
Re: Pluggable Storage - Andres's take
Hi,
Thanks!
On 2019-01-22 11:51:57 +1100, Haribabu Kommi wrote:
> Attached the patch for removal of scan_update_snapshot
> and also the rebased patch of reduction in use of t_tableOid.
I'll soon look at the latter.
> > - consider removing table_gimmegimmeslot()
> > - add substantial docs for every callback
> >
>
> Will work on the above two.
I think it's easier if I do the first, because I can just do it while
rebasing, reducing unnecessary conflicts.
> > While I saw an initial attempt at writing smgl docs for the table AM
> > API, I'm not convinced that's the best approach. I think it might make
> > more sense to have high-level docs in sgml, but then do all the
> > per-callback docs in tableam.h.
> >
>
> OK, I will update the sgml docs accordingly.
> Index AM has per callback docs in the sgml, refactor them also?
I don't think it's a good idea to tackle the index docs at the same time
- this patchset is already humongously large...
> diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 62c5f9fa9f..3dc1444739 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2308,7 +2308,6 @@ static const TableAmRoutine heapam_methods = {
> .scan_begin = heap_beginscan,
> .scan_end = heap_endscan,
> .scan_rescan = heap_rescan,
> - .scan_update_snapshot = heap_update_snapshot,
> .scan_getnextslot = heap_getnextslot,
>
> .parallelscan_estimate = table_block_parallelscan_estimate,
> diff --git a/src/backend/executor/nodeBitmapHeapscan.c
> b/src/backend/executor/nodeBitmapHeapscan.c
> index 59061c746b..b48ab5036c 100644
> --- a/src/backend/executor/nodeBitmapHeapscan.c
> +++ b/src/backend/executor/nodeBitmapHeapscan.c
> @@ -954,5 +954,9 @@ ExecBitmapHeapInitializeWorker(BitmapHeapScanState *node,
> node->pstate = pstate;
>
> snapshot = RestoreSnapshot(pstate->phs_snapshot_data);
> - table_scan_update_snapshot(node->ss.ss_currentScanDesc, snapshot);
> + Assert(IsMVCCSnapshot(snapshot));
> +
> + RegisterSnapshot(snapshot);
> + node->ss.ss_currentScanDesc->rs_snapshot = snapshot;
> + node->ss.ss_currentScanDesc->rs_temp_snap = true;
> }
I was rather thinking that we'd just move this logic into
table_scan_update_snapshot(), without it invoking a callback.
Greetings,
Andres Freund
Re: Pluggable Storage - Andres's take
On Mon, Jan 21, 2019 at 1:01 PM Andres Freund wrote: > Hi, > > On 2018-12-10 18:13:40 -0800, Andres Freund wrote: > > On 2018-11-26 17:55:57 -0800, Andres Freund wrote: > > > FWIW, now that oids are removed, and the tuple table slot abstraction > > > got in, I'm working on rebasing the pluggable storage patchset ontop of > > > that. > > > > I've pushed a version to that to the git tree, including a rebased > > version of zheap: > > https://github.com/anarazel/postgres-pluggable-storage > > https://github.com/anarazel/postgres-pluggable-zheap > > I've pushed the newest, substantially revised, version to the same > repository. Note, that while the newest pluggable-zheap version is newer > than my last email, it's not based on the latest version, and the > pluggable-zheap development is now happening in the main zheap > repository. > Thanks for the new version of patches and changes. > Todo: > - consider removing scan_update_snapshot > Attached the patch for removal of scan_update_snapshot and also the rebased patch of reduction in use of t_tableOid. > - consider removing table_gimmegimmeslot() > - add substantial docs for every callback > Will work on the above two. While I saw an initial attempt at writing smgl docs for the table AM > API, I'm not convinced that's the best approach. I think it might make > more sense to have high-level docs in sgml, but then do all the > per-callback docs in tableam.h. > OK, I will update the sgml docs accordingly. Index AM has per callback docs in the sgml, refactor them also? Regards, Haribabu Kommi Fujitsu Australia 0002-Removal-of-scan_update_snapshot.patch Description: Binary data 0001-Reduce-the-use-of-HeapTuple-t_tableOid.patch Description: Binary data
Re: Allowing extensions to find out the OIDs of their member objects
Andres Freund writes: > On 2019-01-21 18:52:05 -0500, Tom Lane wrote: >> Yes, I said in so many words that I was proposing increasing >> FirstNormalObjectId. I do not think the issues with pg_upgrade itself >> are insoluble --- it would need some historical knowledge about what >> FirstNormalObjectId had been in each prior version, but that's a pretty >> minor problem in the big scheme of things. > Just about every installation uses the oids directly after > FirstNormalObjectId, so that seems fairly painful. It would be painful to change the OIDs of objects that pg_upgrade tries to preserve the OIDs of --- but those are just tables, types, and roles. Everything else would get renumbered automatically during pg_upgrade's dump and reload of the schema. The point of my proposal was that having fixed OIDs for those specific object types might not be necessary for the use-case of generating new FuncExprs and OpExprs. (You would need to look up some associated types, but those would not be hard to get.) An advantage of the OID-mapping proposal is that it can support getting the OIDs of tables and types too. > It'd be more > realistic to create a new zone at UINT32_MAX - something, but that'd > likely still conflict in plenty installations (thanks to toast and WITH > OIDS tables). I'm curious as to how to solve that, if you have a > sketch - less because of this, and more because I think it's not > unlikely that we'll encounter the need for this at some point not too > far away. I have no idea how we'd move table or type OIDs, given that those are potentially on-disk. (Actually ... are table OIDs really on-disk anywhere in user data? Types yes, but tables?) regards, tom lane
Re: explain plans with information about (modified) gucs
On Sun, Jan 20, 2019 at 2:31 PM Tomas Vondra wrote: > Attached is v6 of the patch, adopting "settings" instead of "guc". Ok, looks good. I tried changing config values with the .conf file, alter system, and alter database, and tried a few queries with auto_explain. I made a pass through the config parameters, and don't see anything obviously left out. I have no comments on the source code. One thing stands out: For the docs on auto_explain, all other options have "Only superusers can change this setting.", but log_settings doesn't. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Simplify set of flags used by MyXactFlags
On Mon, Jan 21, 2019 at 02:58:39PM -0300, Alvaro Herrera wrote:
> "... operated on temp namespace" doesn't look good; seems to me to be
> missing an article, for one thing, but really I'm not sure that
> 'namespace' is the term to be using here. I'd say "... operated on
> temporary objects" instead (the plural avoids the need for the article;
> and the user doesn't really care about the namespace itself but rather
> about the objects it contains.)
Thanks for the input. Would you prefer something like the attached
then? I have switched the error message to "temporary objects", and
renamed the flag to XACT_FLAGS_ACCESSEDTEMPOBJECTS.
--
Michael
From bd18d3c8a683a11d0fc89194cc0c60672e62b0e1 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 22 Jan 2019 09:30:01 +0900
Subject: [PATCH] Simplify 2PC restriction handling for temporary objects
There are two flags used to control access to temporary tables and
access to the temporary namespace of a session, however the first
control flag is actually a concept included in the second. This removes
the flag for temporary table tracking, keeping around only the one at
namespace level, renaming it for all kinds of temporary objects.
---
src/backend/access/heap/heapam.c | 4 ++--
src/backend/access/transam/xact.c | 23 +++
src/backend/catalog/namespace.c | 2 +-
src/backend/commands/dropcmds.c | 2 +-
src/backend/commands/extension.c | 2 +-
src/backend/commands/lockcmds.c | 2 +-
src/backend/commands/tablecmds.c | 2 +-
src/include/access/xact.h | 12 +++---
.../expected/test_extensions.out | 2 +-
src/test/regress/expected/temp.out| 22 +-
10 files changed, 30 insertions(+), 43 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 9afbc8228d..205b37e61f 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1144,7 +1144,7 @@ relation_open(Oid relationId, LOCKMODE lockmode)
/* Make note that we've accessed a temporary relation */
if (RelationUsesLocalBuffers(r))
- MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPREL;
+ MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPOBJECTS;
pgstat_initstats(r);
@@ -1194,7 +1194,7 @@ try_relation_open(Oid relationId, LOCKMODE lockmode)
/* Make note that we've accessed a temporary relation */
if (RelationUsesLocalBuffers(r))
- MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPREL;
+ MyXactFlags |= XACT_FLAGS_ACCESSEDTEMPOBJECTS;
pgstat_initstats(r);
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 18467d96d2..8525cc5f8e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2259,13 +2259,18 @@ PrepareTransaction(void)
/* NOTIFY will be handled below */
/*
- * Don't allow PREPARE TRANSACTION if we've accessed a temporary table in
+ * Don't allow PREPARE TRANSACTION if we've accessed a temporary object in
* this transaction. Having the prepared xact hold locks on another
* backend's temp table seems a bad idea --- for instance it would prevent
* the backend from exiting. There are other problems too, such as how to
* clean up the source backend's local buffers and ON COMMIT state if the
* prepared xact includes a DROP of a temp table.
*
+ * Other objects types, like functions, operators or extensions, share the
+ * same restriction as they should not be created, locked or dropped as
+ * this can mess up with this session or even a follow-up session trying
+ * to use the same temporary namespace.
+ *
* We must check this after executing any ON COMMIT actions, because they
* might still access a temp relation.
*
@@ -2273,22 +2278,10 @@ PrepareTransaction(void)
* cases, such as a temp table created and dropped all within the
* transaction. That seems to require much more bookkeeping though.
*/
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPREL))
+ if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPOBJECTS))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot PREPARE a transaction that has operated on temporary tables")));
-
- /*
- * Similarly, PREPARE TRANSACTION is not allowed if the temporary
- * namespace has been involved in this transaction as we cannot allow it
- * to create, lock, or even drop objects within the temporary namespace
- * as this can mess up with this session or even a follow-up session
- * trying to use the same temporary namespace.
- */
- if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot PREPARE a transaction that has operated on temporary namespace")));
+ errmsg("cannot PREPARE a transaction that has operated on temporary objects")));
/*
* Likewise, don't allow PREPARE after pg_export_snapshot. This
Re: Allowing extensions to find out the OIDs of their member objects
Hi, On 2019-01-21 18:52:05 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2019-01-20 18:50:33 -0500, Tom Lane wrote: > >> In [1] I propose that we should allow extensions to get their hands > >> on the ability to transform function calls as per "protransform" and > >> to generate lossy index quals based on items in WHERE clauses. The > >> APIs to give an extension control at the right points seem pretty > >> straightforward, but there's a problem standing in the way of actually > >> doing anything useful once you've got control. In order to either > >> analyze a passed-in clause or generate a new one, the extension will > >> need to know the OIDs of the functions/operators it's working with. > >> And extension objects don't have fixed OIDs. > > > Why does it need to know all its oids, rather than just the one of the > > operation protransform is called for? Am I missing something? And if > > so, why isn't it sufficient to just pass in that oid along with the > > node? > > You would know that the FuncExpr you're given is for the function the > support function is attached to, sure, and you could pull its OID out > of that if you wanted. The problem is that what you want to generate > frequently involves *other* functions or operators. > > The example Paul gave in the other thread is that given > > ST_DWithin(a, b, radius) > > we might wish to generate an indexqual like > > a && expand(b, radius) > > Here, the only OID in easy reach is that of ST_DWithin(). The > problem is to find out the OIDs of && and expand() so we can build > new FuncExpr and OpExpr nodes. I guess I was imagining we'd not create FuncExprs etc, but now that you say it, that'd be absurdely invasive. Thanks. > > Which range are you thinking of handing out here? We use oid ranges as > > proxy for system-object-ness in a number of places, so we can't really > > just hand out ones below FirstNormalObjectId. And we can't really hand > > out any above that, because there'd be conflicts - even if we increased > > FirstNormalObjectId today, there'd be issues with pg_upgrade. > > Yes, I said in so many words that I was proposing increasing > FirstNormalObjectId. I do not think the issues with pg_upgrade itself > are insoluble --- it would need some historical knowledge about what > FirstNormalObjectId had been in each prior version, but that's a pretty > minor problem in the big scheme of things. Just about every installation uses the oids directly after FirstNormalObjectId, so that seems fairly painful. It'd be more realistic to create a new zone at UINT32_MAX - something, but that'd likely still conflict in plenty installations (thanks to toast and WITH OIDS tables). I'm curious as to how to solve that, if you have a sketch - less because of this, and more because I think it's not unlikely that we'll encounter the need for this at some point not too far away. > I'm probably going to push forward with the OID mapping idea instead. > That'll be a bit more painful to use, but I don't see any showstopper > problems ATM. Cool. Greetings, Andres Freund
Re: RelationGetIndexAttrBitmap() small deviation between comment and code
On Tue, 22 Jan 2019 at 12:27, Tom Lane wrote: > > David Rowley writes: > > (This is pretty minor, but I struggled to ignore it) > > In RelationGetIndexAttrBitmap() a comment claims /* We return our > > original working copy for caller to play with */. 3 of the 4 possible > > Bitmapsets follow that comment but for some reason, we make a copy of > > the primary key attrs before returning. This seems both unnecessary > > and also quite out of sync to what all the other Bitmapsets do. I > > don't quite see any reason for doing it so I assume there's none. > > I agree, that's pretty bogus. Will push in a minute. Thanks. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Allowing extensions to find out the OIDs of their member objects
Andres Freund writes: > On 2019-01-20 18:50:33 -0500, Tom Lane wrote: >> In [1] I propose that we should allow extensions to get their hands >> on the ability to transform function calls as per "protransform" and >> to generate lossy index quals based on items in WHERE clauses. The >> APIs to give an extension control at the right points seem pretty >> straightforward, but there's a problem standing in the way of actually >> doing anything useful once you've got control. In order to either >> analyze a passed-in clause or generate a new one, the extension will >> need to know the OIDs of the functions/operators it's working with. >> And extension objects don't have fixed OIDs. > Why does it need to know all its oids, rather than just the one of the > operation protransform is called for? Am I missing something? And if > so, why isn't it sufficient to just pass in that oid along with the > node? You would know that the FuncExpr you're given is for the function the support function is attached to, sure, and you could pull its OID out of that if you wanted. The problem is that what you want to generate frequently involves *other* functions or operators. The example Paul gave in the other thread is that given ST_DWithin(a, b, radius) we might wish to generate an indexqual like a && expand(b, radius) Here, the only OID in easy reach is that of ST_DWithin(). The problem is to find out the OIDs of && and expand() so we can build new FuncExpr and OpExpr nodes. > Which range are you thinking of handing out here? We use oid ranges as > proxy for system-object-ness in a number of places, so we can't really > just hand out ones below FirstNormalObjectId. And we can't really hand > out any above that, because there'd be conflicts - even if we increased > FirstNormalObjectId today, there'd be issues with pg_upgrade. Yes, I said in so many words that I was proposing increasing FirstNormalObjectId. I do not think the issues with pg_upgrade itself are insoluble --- it would need some historical knowledge about what FirstNormalObjectId had been in each prior version, but that's a pretty minor problem in the big scheme of things. What I'm not seeing a solution for is how an extension upgrade script could assign fixed OIDs to existing objects. Since nobody else seems to either see a way to do that, or even like the idea of fixed OIDs at all, I'm probably going to push forward with the OID mapping idea instead. That'll be a bit more painful to use, but I don't see any showstopper problems ATM. regards, tom lane
Re: PostgreSQL vs SQL/XML Standards
Hi, In two places in the XMLTABLE implementation (XmlTableFetchRow, iterating through a nodeset returned by the row_expression, and XmlTableGetValue, going through a nodeset returned by the column_expression), the iteration proceeds in index order through xpathobj->nodesetval->nodeTab. The same happens in the older xml_xpathobjtoxmlarray, producing the array result of the xpath() function. In one finds this unsettling comment: xmlNodePtr *nodeTab; /* array of nodes in no particular order */ So far, no matter what oddball XPath expressions I think up, nodeTab does seem to end up in document order. It would be comforting to document that, but the comment suggests it might not be guaranteed. Are you aware of any statement I might have missed in the libxml docs, where they commit to XPath evaluation returning a nodeset where nodeTab has a predictable order? If there is a statement to that effect somewhere, it might be worth citing in a comment in xml.c. Regards, -Chap Here is a fairly oddball query, generating an XML document with about 3k names in descending order, partitioning those elements into subsets having the same second character of the name, and unioning those back together. You'd sort of expect that to produce a result nodeTab in goofy order if anything could, but no, the results seem verifiably in descending order every time, suggesting the result nodeTab is indeed being put in document order before the XPath evaluator returns. If only they would document that. WITH nodeset_order_check(name, prev_name) AS ( SELECT x, lag(x) OVER (ROWS BETWEEN 1 PRECEDING AND CURRENT ROW) FROM (SELECT string_agg(DISTINCT 'x/n[substring(.,2,1) = "' || substring(proname from 2 for 1) || '"]', '|') FROM pg_proc) AS rowx(rowx), (SELECT XMLELEMENT(NAME x, XMLAGG(XMLELEMENT(NAME n, proname) ORDER BY proname DESC)) FROM pg_proc) AS src(src), XMLTABLE(rowx PASSING src COLUMNS x text PATH '.') ) SELECT every(prev_name >= name IS NOT FALSE) FROM nodeset_order_check; every --- t (1 row)
Re: should ConstraintRelationId ins/upd cause relcache invals?
Hi, On 2019-01-21 20:25:46 -0300, Alvaro Herrera wrote: > Hello > > On 2019-Jan-21, Andres Freund wrote: > > > On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote: > > > On 2019-Jan-21, Tom Lane wrote: > > > > > > > Alvaro Herrera writes: > > > > > > > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I > > > > > posted > > > > > a simplistic for the specific problem I found by calling > > > > > CacheInvalidateRelcache in the problem spot. But I'm wondering if the > > > > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK > > > > > pg_constraint tuples instead, per the attached patch. > > > > > > > > +1, this is safer than expecting retail relcache inval calls to be > > > > added in all the right places. > > > > > > Thanks, pushed. > > > > Given > > https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de > > and the concerns voiced in the thread quoted therein, I'm a bit > > surprised that you just went ahead with this, and backpatched it to boot. > > Sigh. > I don't understand why the arguments about a different patch apply to > this one. The invalidation I added is for pg_constraint, not pg_index. > Tom argues that pg_index can be updated for reasons other than > creation/deletion of the index, and that the relcache entry should not > be invalidated in those cases. So can pg_constraint in some cases, that's not an argument (we'll now e.g. re-compute the table's relcache entry more times than before after e.g. renaming an index - arguably we could fix that by fixing the relcache mechanism to not redundantly re-build). > OTOH I lean towards your side of the argument in the other thread and I > don't quite understand why you gave up on it. Tom didn't respond to > your last argumentat, and neither did you indicate that you were > convinced by Tom's argumentation. I wasn't. My concern isn't that we shouldn't do this, but that we ought to go about this a bit more systematically than just doing this for an individual object type, depending on the mood of the day. Cache invalidation is hard enough to understand already, the more inconsistent we make it harder it is to get it right going forward. > My conclusion is that you disagreed but decided not to push the issue > any further, to get the thing done. What I did here was the same: > just get the thing done. There's a difference between agreeing to disagree and going ahead with a majority solution, and not even bothering to see whether we can find consensus and by not even replying to a message wondering about consistency. Greetings, Andres Freund
Re: should ConstraintRelationId ins/upd cause relcache invals?
Andres Freund writes: > On 2019-01-21 18:14:31 -0500, Tom Lane wrote: >> I don't think that's relevant. The issues there were about whether >> a pg_index row update ought to cause an invalidation of the relcache >> entry for the index's table (not the one for the index, which it >> already takes care of). That seems very questionable to me --- the >> potentially-invalidatable info ought to be in the index's relcache entry, >> not its parent table's entry, IMO. > Well, we've plenty of information about indexes in the table's > relcache. Among other things, the list of indexes, bitmaps of indexed > attributes, which index is the primary key, etc is all maintained > there... So I don't really see a material difference between the > constraint and the index case. The difference is that we don't support index redefinitions that could change any of those properties. regards, tom lane
Re: should ConstraintRelationId ins/upd cause relcache invals?
Hi, On 2019-01-21 18:14:31 -0500, Tom Lane wrote: > Andres Freund writes: > > Given > > https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de > > and the concerns voiced in the thread quoted therein, I'm a bit > > surprised that you just went ahead with this, and backpatched it to boot. > > I don't think that's relevant. The issues there were about whether > a pg_index row update ought to cause an invalidation of the relcache > entry for the index's table (not the one for the index, which it > already takes care of). That seems very questionable to me --- the > potentially-invalidatable info ought to be in the index's relcache entry, > not its parent table's entry, IMO. Well, we've plenty of information about indexes in the table's relcache. Among other things, the list of indexes, bitmaps of indexed attributes, which index is the primary key, etc is all maintained there... So I don't really see a material difference between the constraint and the index case. You can make some arguments about superfluous invals, true. I don't see why rd_indexlist et al is materially different from rd_fkeylist. Greetings, Andres Freund
Re: problems with foreign keys on partitioned tables
Hi Amit, Will you please rebase 0002? Please add your proposed tests cases to it, too. Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Changing SQL Inlining Behaviour (or...?)
> On Jan 21, 2019, at 3:27 PM, Andres Freund wrote: > > Hi, > > On 2019-01-21 15:21:29 -0800, Paul Ramsey wrote: >> As a practical matter, most of the exact-test functions have a >> preamble that checks the bbox, so in the seqscan case having the >> operator along for the ride isn’t any advantage. In any event, if we >> do have exact tests w/o a lossy preamble, we could add that for v12, >> as this renovation won’t be a small one if we go this direction. > > How expensive are the bbox checks in comparison to the exact tests? IOW, > how much of a problem is it to potentially do a bbox check twice? Very very cheap. The geometry object usually has a bbox already instantiated and stored along with the actual coordinates. The exceptions are objects (points, two-vertex lines) that are basically their own boxes anyways. P > > Greetings, > > Andres Freund
Re: Changing SQL Inlining Behaviour (or...?)
Hi, On 2019-01-21 15:21:29 -0800, Paul Ramsey wrote: > As a practical matter, most of the exact-test functions have a > preamble that checks the bbox, so in the seqscan case having the > operator along for the ride isn’t any advantage. In any event, if we > do have exact tests w/o a lossy preamble, we could add that for v12, > as this renovation won’t be a small one if we go this direction. How expensive are the bbox checks in comparison to the exact tests? IOW, how much of a problem is it to potentially do a bbox check twice? Greetings, Andres Freund
Re: RelationGetIndexAttrBitmap() small deviation between comment and code
David Rowley writes: > (This is pretty minor, but I struggled to ignore it) > In RelationGetIndexAttrBitmap() a comment claims /* We return our > original working copy for caller to play with */. 3 of the 4 possible > Bitmapsets follow that comment but for some reason, we make a copy of > the primary key attrs before returning. This seems both unnecessary > and also quite out of sync to what all the other Bitmapsets do. I > don't quite see any reason for doing it so I assume there's none. I agree, that's pretty bogus. Will push in a minute. regards, tom lane
Re: should ConstraintRelationId ins/upd cause relcache invals?
Hello On 2019-Jan-21, Andres Freund wrote: > On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote: > > On 2019-Jan-21, Tom Lane wrote: > > > > > Alvaro Herrera writes: > > > > > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted > > > > a simplistic for the specific problem I found by calling > > > > CacheInvalidateRelcache in the problem spot. But I'm wondering if the > > > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK > > > > pg_constraint tuples instead, per the attached patch. > > > > > > +1, this is safer than expecting retail relcache inval calls to be > > > added in all the right places. > > > > Thanks, pushed. > > Given > https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de > and the concerns voiced in the thread quoted therein, I'm a bit > surprised that you just went ahead with this, and backpatched it to boot. Sigh. I don't understand why the arguments about a different patch apply to this one. The invalidation I added is for pg_constraint, not pg_index. Tom argues that pg_index can be updated for reasons other than creation/deletion of the index, and that the relcache entry should not be invalidated in those cases. Maybe that's right, particularly given that the relcache entry only holds a list of index OIDs, not properties of each index. For foreign keys the relcache entry keeps a lot more than the OIDs; and pg_constraint rows are not updated very much anyway. OTOH I lean towards your side of the argument in the other thread and I don't quite understand why you gave up on it. Tom didn't respond to your last argumentat, and neither did you indicate that you were convinced by Tom's argumentation. My conclusion is that you disagreed but decided not to push the issue any further, to get the thing done. What I did here was the same: just get the thing done. I could just as easily revert this commit and push a lone CacheInvalidateRelcache where it is needed by the other fix I just pushed, but that seemed to me the wrong thing to do. Or I could spend a few hours figuring out test cases that fail in 9.6 with the lack of invalidation, but I don't have those hours and the bug isn't mine anyway. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Changing SQL Inlining Behaviour (or...?)
> On Jan 21, 2019, at 3:17 PM, Andres Freund wrote: > > Hi, > > On 2019-01-19 20:53:55 -0500, Tom Lane wrote: >> I wrote: >>> Paul Ramsey writes: Is there a rule of thumb we can use in costing our wrappers to ensure that they always inline? >> >>> Not really, which is a weak spot of this whole approach. >> >> BTW, it suddenly strikes me that at least for the specific cases you've >> shown in this thread, worrying about forcing inlining despite multiple >> parameter occurrences is solving the wrong problem. As I understand it, >> what you're doing is basically expanding some_operation(x,y) into >> >> x indexable_operator y AND exact_condition(x,y) >> >> where exact_condition(x,y) is semantically identical to >> some_operation(x,y), and the point of the maneuver is to inject >> an indexable operator that implements some lossy form of the >> exact_condition. But this is not an ideal solution: aside >> from the possible cost of evaluating x and y twice, adding >> the indexable_operator is just a dead loss if you don't end up >> with an indexscan on x. >> >> So ISTM what we *really* want is that the wrapper function is >> just an alias for exact_condition() --- which eliminates the >> multiple-evaluation hazards and hence the risk of not inlining --- >> and then teach the planner how to extract the indexable_operator >> when, and only when, it's actually useful for an indexscan. >> >> The machinery for that sort of thing already exists; it's what >> indxpath.c calls a "special index operator". But right now it's >> only applied to some built-in operators such as LIKE. I've had >> a personal TODO item for a very long time to make that ability >> accessible to extensions, but it never rose to the top of the >> queue. Maybe we should be trying to make that happen, instead >> of messing around with dubious changes to the inlining rules. >> >> Does this line of thought seem like it would fix your problem, >> or are there places where the inlining rules are causing you >> issues but the use-case doesn't look like "add a lossy indexable >> operator"? > > Paul, is what Tom calls indexable_operator here just useful for > indexability, or *also* useful to reduce the frequency of invoking the > exact_condition(), which presumably will frequently be much more > expensive to evaluate? With the current scheme, as long as it works, > the indexable fastpath filters out rows quickly for both sequential and > index scans, but I'm not sure that'd work in the transformation scheme > Tom proposes. Sure, the exact_condition could always do the cheaper > pre-check, but that'd be wasted effort in the index scan case, where > that'd presumably already be ruled out. > > Actually an issue, or not? As a practical matter, most of the exact-test functions have a preamble that checks the bbox, so in the seqscan case having the operator along for the ride isn’t any advantage. In any event, if we do have exact tests w/o a lossy preamble, we could add that for v12, as this renovation won’t be a small one if we go this direction. My only concern, as a plebian, is that what he have, hacky or otherwise, works, and the quick’n’dirty solution also works, and the new hotness seems quite complex, relatively speaking. I guess the new hotness also gets us the ability to do selectivity at a function level, which is also something we are interested in the future. so there’s that, but … I’d just be happy to solve the hacky inline problem :) P > Greetings, > > Andres Freund
Re: Changing SQL Inlining Behaviour (or...?)
Hi, On 2019-01-19 20:53:55 -0500, Tom Lane wrote: > I wrote: > > Paul Ramsey writes: > >> Is there a rule of thumb we can use in costing our wrappers to ensure that > >> they always inline? > > > Not really, which is a weak spot of this whole approach. > > BTW, it suddenly strikes me that at least for the specific cases you've > shown in this thread, worrying about forcing inlining despite multiple > parameter occurrences is solving the wrong problem. As I understand it, > what you're doing is basically expanding some_operation(x,y) into > > x indexable_operator y AND exact_condition(x,y) > > where exact_condition(x,y) is semantically identical to > some_operation(x,y), and the point of the maneuver is to inject > an indexable operator that implements some lossy form of the > exact_condition. But this is not an ideal solution: aside > from the possible cost of evaluating x and y twice, adding > the indexable_operator is just a dead loss if you don't end up > with an indexscan on x. > > So ISTM what we *really* want is that the wrapper function is > just an alias for exact_condition() --- which eliminates the > multiple-evaluation hazards and hence the risk of not inlining --- > and then teach the planner how to extract the indexable_operator > when, and only when, it's actually useful for an indexscan. > > The machinery for that sort of thing already exists; it's what > indxpath.c calls a "special index operator". But right now it's > only applied to some built-in operators such as LIKE. I've had > a personal TODO item for a very long time to make that ability > accessible to extensions, but it never rose to the top of the > queue. Maybe we should be trying to make that happen, instead > of messing around with dubious changes to the inlining rules. > > Does this line of thought seem like it would fix your problem, > or are there places where the inlining rules are causing you > issues but the use-case doesn't look like "add a lossy indexable > operator"? Paul, is what Tom calls indexable_operator here just useful for indexability, or *also* useful to reduce the frequency of invoking the exact_condition(), which presumably will frequently be much more expensive to evaluate? With the current scheme, as long as it works, the indexable fastpath filters out rows quickly for both sequential and index scans, but I'm not sure that'd work in the transformation scheme Tom proposes. Sure, the exact_condition could always do the cheaper pre-check, but that'd be wasted effort in the index scan case, where that'd presumably already be ruled out. Actually an issue, or not? Greetings, Andres Freund
Re: should ConstraintRelationId ins/upd cause relcache invals?
Andres Freund writes: > Given > https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de > and the concerns voiced in the thread quoted therein, I'm a bit > surprised that you just went ahead with this, and backpatched it to boot. I don't think that's relevant. The issues there were about whether a pg_index row update ought to cause an invalidation of the relcache entry for the index's table (not the one for the index, which it already takes care of). That seems very questionable to me --- the potentially-invalidatable info ought to be in the index's relcache entry, not its parent table's entry, IMO. Here, however, it's clear which relcache entry is dependent on those pg_constraint rows (as long as Alvaro got it right about whether to inval conrelid or confrelid ...), and that it is indeed so dependent. regards, tom lane
Re: problems with foreign keys on partitioned tables
Pushed now, thanks. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Allowing extensions to find out the OIDs of their member objects
Hi, On 2019-01-20 18:50:33 -0500, Tom Lane wrote: > In [1] I propose that we should allow extensions to get their hands > on the ability to transform function calls as per "protransform" and > to generate lossy index quals based on items in WHERE clauses. The > APIs to give an extension control at the right points seem pretty > straightforward, but there's a problem standing in the way of actually > doing anything useful once you've got control. In order to either > analyze a passed-in clause or generate a new one, the extension will > need to know the OIDs of the functions/operators it's working with. > And extension objects don't have fixed OIDs. Why does it need to know all its oids, rather than just the one of the operation protransform is called for? Am I missing something? And if so, why isn't it sufficient to just pass in that oid along with the node? > A larger issue is whether "hand out some OIDs on-demand" is a > sustainable strategy. I think that it is, if we encourage extensions > to assign fixed OIDs only to objects they really need to. In thirty-ish > years of core PG development, we've only used up ~4200 fixed OIDs, > and a lot of those are for functions that probably don't really need > fixed OIDs but got one because we give one to every built-in function. > However, if there's a big land rush to claim large chunks of OIDs, > we might have a problem. I'm not sure that "30 years" argument holds that much power - we've surely reused oids. And core PG is going to be much more conservative than any sort of external user. Which range are you thinking of handing out here? We use oid ranges as proxy for system-object-ness in a number of places, so we can't really just hand out ones below FirstNormalObjectId. And we can't really hand out any above that, because there'd be conflicts - even if we increased FirstNormalObjectId today, there'd be issues with pg_upgrade. Greetings, Andres Freund
RelationGetIndexAttrBitmap() small deviation between comment and code
(This is pretty minor, but I struggled to ignore it) In RelationGetIndexAttrBitmap() a comment claims /* We return our original working copy for caller to play with */. 3 of the 4 possible Bitmapsets follow that comment but for some reason, we make a copy of the primary key attrs before returning. This seems both unnecessary and also quite out of sync to what all the other Bitmapsets do. I don't quite see any reason for doing it so I assume there's none. The attached removes the bms_copy() and just returns the set that's already been built in the same memory context. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services relationgetindexattrbitmap_fix.patch Description: Binary data
Re: should ConstraintRelationId ins/upd cause relcache invals?
Hi, On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote: > On 2019-Jan-21, Tom Lane wrote: > > > Alvaro Herrera writes: > > > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted > > > a simplistic for the specific problem I found by calling > > > CacheInvalidateRelcache in the problem spot. But I'm wondering if the > > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK > > > pg_constraint tuples instead, per the attached patch. > > > > +1, this is safer than expecting retail relcache inval calls to be > > added in all the right places. > > Thanks, pushed. Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de and the concerns voiced in the thread quoted therein, I'm a bit surprised that you just went ahead with this, and backpatched it to boot. Greetings, Andres Freund
Re: should ConstraintRelationId ins/upd cause relcache invals?
On 2019-Jan-21, Tom Lane wrote: > Alvaro Herrera writes: > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted > > a simplistic for the specific problem I found by calling > > CacheInvalidateRelcache in the problem spot. But I'm wondering if the > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK > > pg_constraint tuples instead, per the attached patch. > > +1, this is safer than expecting retail relcache inval calls to be > added in all the right places. Thanks, pushed. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: COPY FROM WHEN condition
On 1/21/19 7:51 PM, Andres Freund wrote:
> Hi,
>
> On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote:
>>
>>
>> On 1/21/19 4:33 AM, Tomas Vondra wrote:
>>>
>>>
>>> On 1/21/19 3:12 AM, Andres Freund wrote:
On 2019-01-20 18:08:05 -0800, Andres Freund wrote:
> On 2019-01-20 21:00:21 -0500, Tomas Vondra wrote:
>>
>>
>> On 1/20/19 8:24 PM, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2019-01-20 00:24:05 +0100, Tomas Vondra wrote:
On 1/14/19 10:25 PM, Tomas Vondra wrote:
> On 12/13/18 8:09 AM, Surafel Temesgen wrote:
>>
>>
>> On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra
>> mailto:tomas.von...@2ndquadrant.com>>
>> wrote:
>>
>>
>> Can you also update the docs to mention that the functions
>> called from
>> the WHERE clause does not see effects of the COPY itself?
>>
>>
>> /Of course, i also add same comment to insertion method selection
>> /
>
> FWIW I've marked this as RFC and plan to get it committed this week.
>
Pushed, thanks for the patch.
>>>
>>> While rebasing the pluggable storage patch ontop of this I noticed that
>>> the qual appears to be evaluated in query context. Isn't that a bad
>>> idea? ISMT it should have been evaluated a few lines above, before the:
>>>
>>> /* Triggers and stuff need to be invoked in query
>>> context. */
>>> MemoryContextSwitchTo(oldcontext);
>>>
>>> Yes, that'd require moving the ExecStoreHeapTuple(), but that seems ok?
>>>
>>
>> Yes, I agree. It's a bit too late for me to hack and push stuff, but I'll
>> fix that tomorrow.
>
> NP. On second thought, the problem is probably smaller than I thought at
> first, because ExecQual() switches to the econtext's per-tuple memory
> context. But it's only reset once for each batch, so there's some
> wastage. At least worth a comment.
I'm tired, but perhaps its actually worse - what's being reset currently
is the ESTate's per-tuple context:
if (nBufferedTuples == 0)
{
/*
* Reset the per-tuple exprcontext. We can only do this
if the
* tuple buffer is empty. (Calling the context the
per-tuple
* memory context is a bit of a misnomer now.)
*/
ResetPerTupleExprContext(estate);
}
but the quals are evaluated in the ExprContext's:
ExecQual(ExprState *state, ExprContext *econtext)
...
ret = ExecEvalExprSwitchContext(state, econtext, );
which is created with:
/* Get an EState's per-output-tuple exprcontext, making it if first use */
#define GetPerTupleExprContext(estate) \
((estate)->es_per_tuple_exprcontext ? \
(estate)->es_per_tuple_exprcontext : \
MakePerTupleExprContext(estate))
and creates its own context:
/*
* Create working memory for expression evaluation in this context.
*/
econtext->ecxt_per_tuple_memory =
AllocSetContextCreate(estate->es_query_cxt,
"ExprContext",
ALLOCSET_DEFAULT_SIZES);
so this is currently just never reset.
>>>
>>> Actually, no. The ResetPerTupleExprContext boils down to
>>>
>>> MemoryContextReset((econtext)->ecxt_per_tuple_memory)
>>>
>>> and ExecEvalExprSwitchContext does this
>>>
>>> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>>>
>>> So it's resetting the right context, although only on batch boundary.
>
Seems just using ExecQualAndReset() ought to be sufficient?
>>>
>>> That may still be the right thing to do.
>>>
>>
>> Actually, no, because that would reset the context far too early (and
>> it's easy to trigger segfaults). So the reset would have to happen after
>> processing the row, not this early.
>
> Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting
> up the pluggable storage patch into individual pieces...
>
>
>> But I think the current behavior is actually OK, as it matches what we
>> do for defexprs. And the comment before ResetPerTupleExprContext says this:
>>
>> /*
>> * Reset the per-tuple exprcontext. We can only do this if the
>> * tuple buffer is empty. (Calling the context the per-tuple
>> * memory context is a bit of a misnomer now.)
>> */
>>
>> So the per-tuple context is not quite per-tuple anyway. Sure, we might
>> rework that but I don't think that's an issue in this patch.
>
> I'm *not* convinced by this. I think it's bad enough that we do this for
> normal COPY, but for WHEN, we could end up
is there a reason we don't xlog index free space?
I knew that XLogRecordPageWithFreeSpace() is only called by heapam.c, but I figured indexes had their own logic to xlog free space. While investigating something else I found that brin indexes, at least, on the secondary have no FSMs. Is this deliberate? -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: WIP: Avoid creation of the free space map for small tables
On Mon, Jan 21, 2019 at 6:32 AM Amit Kapila wrote: > So we won't allow transfer of FSM files if their size is below > HEAP_FSM_CREATION_THRESHOLD. What will be its behavior in link mode? > It seems that the old files will remain there. Will it create any > problem when we try to create the files via the new server, can you > once test this case? I tried upgrading in --link mode, and on the new cluster, enlarging the table past the threshold causes a new FSM to be created as expected. > Also, another case to think in this regard is the upgrade for standby > servers, if you read below paragraph from the user manual [1], you > will see what I am worried about? > > "What this does is to record the links created by pg_upgrade's link > mode that connect files in the old and new clusters on the primary > server. It then finds matching files in the standby's old cluster and > creates links for them in the standby's new cluster. Files that were > not linked on the primary are copied from the primary to the standby. > (They are usually small.)" > > [1] - https://www.postgresql.org/docs/devel/pgupgrade.html Trying this, I ran into a couple problems. I'm probably doing something wrong, but I can't help but think there's a pg_upgrade bug/feature I'm unaware of: I set up my test to have primary directory data1 and for the secondary standby/data1. I instructed pg_upgrade to upgrade data1 into data1u, and I tried the rsync recipe in the docs quoted above, and the upgraded standby wouldn't go into recovery. While debugging that, I found surprisingly that pg_upgrade also went further and upgraded standby/data1 into standby/data1u. I tried deleting standby/data1u before running the rsync command and still nothing. Because the upgraded secondary is non-functional, I can't really answer your question. Not sure if this is normal, but the pg_upgraded new cluster no longer had the replication slot. Re-adding it didn't allow my upgraded secondary to go into recovery, either. (I made sure to copy the recovery settings, so that can't be the problem) -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Fixing findDependentObjects()'s dependency on scan order (regressions in DROP diagnostic messages)
On Fri, Jan 18, 2019 at 4:06 PM Peter Geoghegan wrote: > > * Objects-to-drop output from DROP ROLE is still unstable. I suppose > > this would be fixed by also doing sorting in that code path, but > > I've not looked into it. > > The nbtree patch is not dependent on doing better here, since the > order here is merely unstable, without leading to incorrect diagnostic > messages. I can just change the regress output mechanically. That > said, I cannot be sure that the instability won't become more of an > annoyance with heap TID being treated as a tie-breaker attribute > within nbtree. It's probably fine to reserve the option to do better > here, and do so if and when it becomes clear that it matters. I defer > to you. Good news: Testing my patch (with the hacky pg_depend tie-breaker commit removed) on top of your recent commit f1ad067f confirms the only "real" remaining problem is with two tests that relate to partitioning (the multiple DEPENDENCY_INTERNAL_AUTO entry issue). It seems likely that objects-to-drop output from DROP ROLE can remain "unstable" without bothering anybody -- I've run "make -Otarget -j10 -s check-world" with the same source tree multiple times, and have yet to see a test failure. In practice *all* of the instability that's of practical concern (that could cause buildfarm failures) related to the two pg_depend indexes. It very much looks that way, at least -- only the buildfarm can confirm this. I attach a patch that shows how I'll adjust the harmless objects-to-drop output from DROP ROLE to make relevant tests pass -- the scope of changes is very limited, and the changes are harmless/mechanical. Separately, I attach the patch that I'd have to use to paper over the partitioning/DEPENDENCY_INTERNAL_AUTO issue (a patch that makes the regression tests actively ignore visible manifestations of the DEPENDENCY_INTERNAL_AUTO bug). As I said, this confirms that there are only two "real" remaining tests that fail. Thanks -- Peter Geoghegan Paper-over-unacceptable-regression-test-failures.patch Description: Binary data Add-harmless-regression-test-changes.patch Description: Binary data
Re: Hint and detail punctuation
On Wed, Dec 5, 2018 at 05:22:25PM +0100, Daniel Gustafsson wrote: > While looking at error messages downstream, I noticed a few hints and details > in postgres which aren’t punctuated as per the style guide. The attached > patch > fixes the ones where it seemed reasonable to end with a period. FYI, this was applied: commit 730422afcdb6784bbe20efc65de72156d470b0c4 Author: Michael Paquier Date: Fri Dec 7 07:47:42 2018 +0900 Fix some errhint and errdetail strings missing a period As per the error message style guide of the documentation, those should be full sentences. Author: Daniel Gustafsson Reviewed-by: Michael Paquier, Álvaro Herrera Discussion: https://1e8d49b4-16bc-4420-b4ed-58501d9e0...@yesql.se -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: Alternative to \copy in psql modelled after \g
Bonsoir Daniel,
sql> SELECT 1 \g /BAD
/BAD: Permission denied
sql> \echo :ERROR
false
That seems broken, because it's pointless to leave out a class of errors
from ERROR.
Yep. That is my point, but fixing it requires to decide whether it is okay
to change ERROR documentation and behavior, and ISTM that there is some
legit case to have the SQL_ERROR information independently.
Now if you download data with SELECT or COPY and we can't even
create the file, how is that a good idea to intentionally have the
script fail to detect it? What purpose does it satisfy?
It means that the client knows that the SQL command, and possible
associated side effects, were executed server-side, and that if we are in
a transaction it is still going on:
BEGIN;
UPDATE pgbench_branches SET bbalance = bbalance + 1 RETURNING * \g /bad
// the update is performed, the transaction is not rollbacked,
// *but* the output file was not written, "COMMIT" save changes.
The documentation states that ERROR is about SQL, not psql internal stuff:
ERROR true if the last SQL query failed, false if it succeeded.
See also SQLSTATE.
ERROR is set by SetResultVariables(PGresult *results, bool success)
and takes the value of "success", ignoring the PGresult.
Yes and no: "success" pretty often currently depends on PGresult, eg:
if (PQresultStatus(results) != PGRES_COMMAND_OK) {
...
SetResultVariables(results, false);
results = PQexec(pset.db, buf.data);
OK = AcceptResult(results); // true if SQL is ok
...
SetResultVariables(results, OK);
results = PQexec(pset.db, buf.data);
OK = AcceptResult(results) &&
(PQresultStatus(results) == PGRES_COMMAND_OK);
if (!OK)
SetResultVariables(results, OK);
So why is ERROR independant of the SQL result, relatively to your
claim that it should never reflect anything else?
AFAICS I'm not claiming that, on the contrary I'm basically ok with
changing ERROR documentation and implementation (what I called option 1),
although it would have to pass through committers, *AND* I'm still
thinking that having a separate access to whether the SQL failed or not is
of interest, so there is an argument to add a SQL_ERROR which reflects the
current documentation, if not fully the implementation.
--
Fabien .
Re: Protect syscache from bloating with negative cache entries
On Fri, Jan 18, 2019 at 05:09:41PM -0800, Andres Freund wrote: > Hi, > > On 2019-01-18 19:57:03 -0500, Robert Haas wrote: > > On Fri, Jan 18, 2019 at 4:23 PM and...@anarazel.de > > wrote: > > > My proposal for this was to attach a 'generation' to cache entries. Upon > > > access cache entries are marked to be of the current > > > generation. Whenever existing memory isn't sufficient for further cache > > > entries and, on a less frequent schedule, triggered by a timer, the > > > cache generation is increased and th new generation's "creation time" is > > > measured. Then generations that are older than a certain threshold are > > > purged, and if there are any, the entries of the purged generation are > > > removed from the caches using a sequential scan through the cache. > > > > > > This outline achieves: > > > - no additional time measurements in hot code paths > > > - no need for a sequential scan of the entire cache when no generations > > > are too old > > > - both size and time limits can be implemented reasonably cheaply > > > - overhead when feature disabled should be close to zero > > > > Seems generally reasonable. The "whenever existing memory isn't > > sufficient for further cache entries" part I'm not sure about. > > Couldn't that trigger very frequently and prevent necessary cache size > > growth? > > I'm thinking it'd just trigger a new generation, with it's associated > "creation" time (which is cheap to acquire in comparison to creating a > number of cache entries) . Depending on settings or just code policy we > can decide up to which generation to prune the cache, using that > creation time. I'd imagine that we'd have some default cache-pruning > time in the minutes, and for workloads where relevant one can make > sizing configurations more aggressive - or something like that. OK, so it seems everyone likes the idea of a timer. The open questions are whether we want multiple epochs, and whether we want some kind of size trigger. With only one time epoch, if the timer is 10 minutes, you could expire an entry after 10-19 minutes, while with a new epoch every minute and 10-minute expire, you can do 10-11 minute precision. I am not sure the complexity is worth it. For a size trigger, should removal be effected by how many expired cache entries there are? If there were 10k expired entries or 50, wouldn't you want them removed if they have not been accessed in X minutes? In the worst case, if 10k entries were accessed in a query and never accessed again, what would the ideal cleanup behavior be? Would it matter if it was expired in 10 or 19 minutes? Would it matter if there were only 50 entries? -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line
Hi Andrey,
Thank you for the review! I have updated the patch according to your
comments and remarks. Please, find new version attached.
On 2019-01-07 12:12, Andrey Borodin wrote:
Here are some my notes:
1. RestoreArchivedWAL() shares a lot of code with
RestoreArchivedFile(). Is it possible\viable to refactor and extract
common part?
I am not sure, but I guess that differences in errors reporting and
points 2-3 are enough to leave new RestoreArchivedWAL() apart. There
are not many common parts left.
2. IMV pg_rewind with %r restore_command should fail. %r is designed
to clean archive from WALs, nothing should be deleted in case of
fetching WALs for rewind. Last restartpoint has no meaning during
rewind. Or does it? If so, let's comment about it.
Yes, during rewind we need the last common checkpoint, not restart
point.
Taking into account that Michael pointed me to this place too and I
cannot
propose a real-life example of restore_command with %r to use in
pg_rewind,
I decided to add an exception in such a case. So now pg_rewind will fail
with error.
3. RestoreArchivedFile() checks for signals, is it done by pg_rewind
elsewhere?
There is a comment part inside RestoreArchivedFile():
* However, if the failure was due to any sort of signal, it's best to
* punt and abort recovery. (If we "return false" here, upper levels
will
* assume that recovery is complete and start up the database!)
In other words, there is a possibility to start up the database assuming
that recovery went well, while actually it was terminated by signal. It
happens since failure is expected during the recovery, so some kind of
ambiguity occurs, which we trying to solve checking for termination
signals.
In contrast, we are looking in the archive for each missing WAL file
during
pg_rewind and if we failed to restore it by any means rewind will fail
indifferent of was it due to the termination signal or file is actually
missing in the archive. Thus, there no ambiguity occurs and we do not
need
to check for signals here.
That is how I understand it. Probably someone can explain why I am
wrong.
4. No documentation is updated
I have updated docs in order to reflect the new functionality as well.
5. -R takes precedence over -r without notes. Shouldn't we complain?
Or may be we should take one from config, iif nothing found use -R?
I do not think it is worth of additional complexity and we could expect,
that end-users know, what they want to use–either -r or -R–so I added
an error message due to the conflicting options.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
From cc64d7943e220ba6434b76db63a579ccb547517a Mon Sep 17 00:00:00 2001
From: Alexey Kondratov
Date: Fri, 21 Dec 2018 14:00:30 +0300
Subject: [PATCH] pg_rewind: options to use restore_command from
postgresql.conf or command line.
---
doc/src/sgml/ref/pg_rewind.sgml | 30 +-
src/backend/Makefile | 4 +-
src/backend/commands/extension.c | 1 +
src/backend/utils/misc/Makefile | 8 -
src/backend/utils/misc/guc.c | 434 +--
src/bin/pg_rewind/Makefile| 2 +-
src/bin/pg_rewind/RewindTest.pm | 96 +++-
src/bin/pg_rewind/parsexlog.c | 180 +-
src/bin/pg_rewind/pg_rewind.c | 100 +++-
src/bin/pg_rewind/pg_rewind.h | 12 +-
src/bin/pg_rewind/t/001_basic.pl | 4 +-
src/bin/pg_rewind/t/002_databases.pl | 4 +-
src/bin/pg_rewind/t/003_extrafiles.pl | 4 +-
src/common/Makefile | 9 +-
src/{backend/utils/misc => common}/guc-file.l | 514 --
src/include/common/guc-file.h | 50 ++
src/include/utils/guc.h | 39 +-
src/tools/msvc/Mkvcbuild.pm | 2 +-
src/tools/msvc/clean.bat | 2 +-
19 files changed, 987 insertions(+), 508 deletions(-)
rename src/{backend/utils/misc => common}/guc-file.l (60%)
create mode 100644 src/include/common/guc-file.h
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 53a64ee29e..34046d6404 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -67,8 +67,10 @@ PostgreSQL documentation
ancestor. In the typical failover scenario where the target cluster was
shut down soon after the divergence, this is not a problem, but if the
target cluster ran for a long time after the divergence, the old WAL
- files might no longer be present. In that case, they can be manually
- copied from the WAL archive to the pg_wal directory, or
+ files might no longer be present. In that case, they can be automatically
+ copied by pg_rewind from the WAL archive to the
+ pg_wal directory if either -r or
+ -R options are specified, or
fetched
Re: Thread-unsafe coding in ecpg
Andres Freund writes: > We could just refuse to support thread safety on mingw if that's not > supported? Or is that too aggressive? Nah, we already had that discussion upthread. Given the lack of prior complaints, we shouldn't break cases that are working today. For instance, as long as setlocale() isn't actively thread-unsafe and you are running with LC_NUMERIC=C as the prevailing locale, the existing code doesn't pose a hazard even in a threaded app. Forcing users for whom that's true to --disable-thread-safety would turn an OK situation into a broken one. regards, tom lane
Re: Thread-unsafe coding in ecpg
"Joshua D. Drake" writes: > On 1/21/19 12:05 PM, Tom Lane wrote: >> Is there a newer version of mingw that does have this functionality? > Apparently this can be done with thee 64bit version: > https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw Hmm, the followup question makes it sound like it still didn't work :-(. However, since the mingw build is autoconf-based, seems like we can install a configure check instead of guessing. Will make it so. Task for somebody else: run a MinGW64 buildfarm member. regards, tom lane
Re: Thread-unsafe coding in ecpg
Hi, On 2019-01-21 15:05:23 -0500, Tom Lane wrote: > Andres Freund writes: > > Seems jacana might not have like this change? > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 > > Hmm. So mingw doesn't provide access to _configthreadlocale(). > That's unfortunate, at least if we think that mingw is still a viable > production platform, because it means we can't make ecpg thread-safe > on that platform. > > Is there a newer version of mingw that does have this functionality? > I'm not sure whether to install a version check or just assume that > it's never there. It does seem like newer versions do have it: https://sourceforge.net/p/mingw-w64/mailman/message/34765722/ https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/_configthreadlocale.c We could just refuse to support thread safety on mingw if that's not supported? Or is that too aggressive? Greetings, Andres Freund
Re: Thread-unsafe coding in ecpg
On 1/21/19 12:05 PM, Tom Lane wrote: Andres Freund writes: Seems jacana might not have like this change? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Hmm. So mingw doesn't provide access to _configthreadlocale(). That's unfortunate, at least if we think that mingw is still a viable production platform, because it means we can't make ecpg thread-safe on that platform. Is there a newer version of mingw that does have this functionality? I'm not sure whether to install a version check or just assume that it's never there. Apparently this can be done with thee 64bit version: https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw JD regards, tom lane -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Re: should ConstraintRelationId ins/upd cause relcache invals?
Alvaro Herrera writes: > While working on bugfixes for FK problems in partitioned tables, I came > across some behavior that appears to stem from our inclusion of foreign > keys in relcache, without sufficient care for invalidating the relcache > entries when the foreign key set for the table changes. (Namely, a > partition retains its relcache entry with no FKs when an FK is added to > the parent table, leading a DELETE to skip running action triggers). Ooops. > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted > a simplistic for the specific problem I found by calling > CacheInvalidateRelcache in the problem spot. But I'm wondering if the > correct fix isn't to have CacheInvalidateHeapTuple deal with FK > pg_constraint tuples instead, per the attached patch. +1, this is safer than expecting retail relcache inval calls to be added in all the right places. regards, tom lane
Re: Thread-unsafe coding in ecpg
Andres Freund writes: > Seems jacana might not have like this change? > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Hmm. So mingw doesn't provide access to _configthreadlocale(). That's unfortunate, at least if we think that mingw is still a viable production platform, because it means we can't make ecpg thread-safe on that platform. Is there a newer version of mingw that does have this functionality? I'm not sure whether to install a version check or just assume that it's never there. regards, tom lane
Re: Thread-unsafe coding in ecpg
On 2019-01-21 12:09:30 -0500, Tom Lane wrote: > "Tsunakawa, Takayuki" writes: > > From: Tom Lane [mailto:t...@sss.pgh.pa.us] > >> So like this ... > > > How quick! Thank you. I've reviewed the code for both Unix and Windows, > > and it looks OK. I haven't built the patch, but expect the buildfarm to do > > the test. > > Thanks for reviewing! I've pushed this now (to HEAD only for the moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Seems jacana might not have like this change? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Greetings, Andres Freund
Re: should ConstraintRelationId ins/upd cause relcache invals?
Hi,
On 2019-01-21 16:27:50 -0300, Alvaro Herrera wrote:
> While working on bugfixes for FK problems in partitioned tables, I came
> across some behavior that appears to stem from our inclusion of foreign
> keys in relcache, without sufficient care for invalidating the relcache
> entries when the foreign key set for the table changes. (Namely, a
> partition retains its relcache entry with no FKs when an FK is added to
> the parent table, leading a DELETE to skip running action triggers).
>
> At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> a simplistic for the specific problem I found by calling
> CacheInvalidateRelcache in the problem spot. But I'm wondering if the
> correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> pg_constraint tuples instead, per the attached patch. Why does this not
> lead to stale cache problems elsewhere?
>
> FKs were added to relcache entries by commit 100340e2dcd0 ("Restore
> foreign-key-aware estimation of join relation sizes"), so CCing Tom and
> Tomas.
I wondered about the same in
https://www.postgresql.org/message-id/20180628150209.n2qch5jtn3vt2xaa%40alap3.anarazel.de
, just about pg_index, but people didn't like it much.
Greetings,
Andres Freund
should ConstraintRelationId ins/upd cause relcache invals?
Hello
While working on bugfixes for FK problems in partitioned tables, I came
across some behavior that appears to stem from our inclusion of foreign
keys in relcache, without sufficient care for invalidating the relcache
entries when the foreign key set for the table changes. (Namely, a
partition retains its relcache entry with no FKs when an FK is added to
the parent table, leading a DELETE to skip running action triggers).
At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
a simplistic for the specific problem I found by calling
CacheInvalidateRelcache in the problem spot. But I'm wondering if the
correct fix isn't to have CacheInvalidateHeapTuple deal with FK
pg_constraint tuples instead, per the attached patch. Why does this not
lead to stale cache problems elsewhere?
FKs were added to relcache entries by commit 100340e2dcd0 ("Restore
foreign-key-aware estimation of join relation sizes"), so CCing Tom and
Tomas.
--
Álvaro HerreraPostgreSQL Expert, https://www.2ndQuadrant.com/
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 80d7a76e24..b9f698ef2c 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -52,10 +52,11 @@
* catcaches may need invalidation for a given tuple.
*
* Also, whenever we see an operation on a pg_class, pg_attribute, or
* pg_index tuple, we register a relcache flush operation for the relation
* described by that tuple (as specified in CacheInvalidateHeapTuple()).
+ * Likewise for pg_constraint tuples for foreign keys on relations.
*
* We keep the relcache flush requests in lists separate from the catcache
* tuple flush requests. This allows us to issue all the pending catcache
* flushes before we issue relcache flushes, which saves us from loading
* a catcache tuple during relcache load only to flush it again right away.
@@ -98,10 +99,11 @@
#include
#include "access/htup_details.h"
#include "access/xact.h"
#include "catalog/catalog.h"
+#include "catalog/pg_constraint.h"
#include "miscadmin.h"
#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/catcache.h"
#include "utils/inval.h"
@@ -1201,10 +1203,27 @@ CacheInvalidateHeapTuple(Relation relation,
* shared catalogs can't have such updates.
*/
relationId = indextup->indexrelid;
databaseId = MyDatabaseId;
}
+ else if (tupleRelId == ConstraintRelationId)
+ {
+ Form_pg_constraint constrtup = (Form_pg_constraint) GETSTRUCT(tuple);
+
+ /*
+ * Foreign keys are part of relcache entries, too, so send out an
+ * inval for the table that the FK applies to.
+ */
+ if (constrtup->contype == CONSTRAINT_FOREIGN &&
+ OidIsValid(constrtup->conrelid))
+ {
+ relationId = constrtup->conrelid;
+ databaseId = MyDatabaseId;
+ }
+ else
+ return;
+ }
else
return;
/*
* Yes. We need to register a relcache invalidation event.
Re: House style for DocBook documentation?
On 01/21/19 13:14, Joshua D. Drake wrote: >> Of course, the text would also be clickable, right? I think putting the >> URL in a footnote is good in that case; it works both on screen and on >> paper, which should alleviate JD's concerns. > > Yeah I could see that. I thought about that but was wondering if it was It looks like the easiest way to integrate such a behavior into the current Makefile would be not as a separate DocBook->DocBook transform in advance, but simply by editing the existing stylesheet-fo.xsl that produces the FO input for generating the PDF. That would mean learning some FO, which I've wanted to do for a while, but haven't yet, so it stops looking like something I might experiment with this afternoon. OTOH, it could mean more flexibility in how the presentation should look. I note in passing that the google result [1] is nonempty -Chap [1] https://www.google.com/search?q=xsl-fo+qr+code
Re: What to name the current heap after pluggable storage / what to rename?
On 2019-01-18 14:19:41 -0800, Andres Freund wrote: > Hi, > > On 2019-01-16 08:20:37 -0800, Andres Freund wrote: > > On January 16, 2019 8:08:09 AM PST, Robert Haas > > wrote: > > >On Tue, Jan 15, 2019 at 10:23 PM Haribabu Kommi > > > wrote: > > >> access/relation.[c|h] name is fine. Or how about access/rel.[c|h], > > >> because nodes/relation.h is related to planner. utils/rel.h is some > > >how > > >> related to relation caches. > > > > > >Insofar as we can reasonably do so, I'd rather pick unique names for > > >header files. I know that there's no law that rules out having both > > >nodes/relation.h and access/relation.h, or likewise utils/rel.h and > > >access/rel.h; the computer won't be confused. But it might create > > >some confusion among human beings, so my vote is for avoiding that > > >sort of thing if we can. > > > > I prefer that too - if the new name isn't worse enough to make it hard > > to remember. I'd welcome suggestions that don't conflict... > > Unless somebody comes up with a better suggestion I'm planning to press > ahead with this one. It's large enough to be a bit of a pain to maintain > over time... I'm absolutely not wedded to access/relation.h, so I'm > happy with another good suggestion, or somebody revising it subsequently. And pushed. If somebody is interested in renaming/splitting nodes/relation.h, I think that'd be good, but if not, it's also not terrible. - Andres
Re: COPY FROM WHEN condition
Hi,
On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote:
>
>
> On 1/21/19 4:33 AM, Tomas Vondra wrote:
> >
> >
> > On 1/21/19 3:12 AM, Andres Freund wrote:
> >> On 2019-01-20 18:08:05 -0800, Andres Freund wrote:
> >>> On 2019-01-20 21:00:21 -0500, Tomas Vondra wrote:
>
>
> On 1/20/19 8:24 PM, Andres Freund wrote:
> > Hi,
> >
> > On 2019-01-20 00:24:05 +0100, Tomas Vondra wrote:
> >> On 1/14/19 10:25 PM, Tomas Vondra wrote:
> >>> On 12/13/18 8:09 AM, Surafel Temesgen wrote:
>
>
> On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>>
> wrote:
>
>
> Can you also update the docs to mention that the functions
> called from
> the WHERE clause does not see effects of the COPY itself?
>
>
> /Of course, i also add same comment to insertion method selection
> /
> >>>
> >>> FWIW I've marked this as RFC and plan to get it committed this week.
> >>>
> >>
> >> Pushed, thanks for the patch.
> >
> > While rebasing the pluggable storage patch ontop of this I noticed that
> > the qual appears to be evaluated in query context. Isn't that a bad
> > idea? ISMT it should have been evaluated a few lines above, before the:
> >
> > /* Triggers and stuff need to be invoked in query
> > context. */
> > MemoryContextSwitchTo(oldcontext);
> >
> > Yes, that'd require moving the ExecStoreHeapTuple(), but that seems ok?
> >
>
> Yes, I agree. It's a bit too late for me to hack and push stuff, but I'll
> fix that tomorrow.
> >>>
> >>> NP. On second thought, the problem is probably smaller than I thought at
> >>> first, because ExecQual() switches to the econtext's per-tuple memory
> >>> context. But it's only reset once for each batch, so there's some
> >>> wastage. At least worth a comment.
> >>
> >> I'm tired, but perhaps its actually worse - what's being reset currently
> >> is the ESTate's per-tuple context:
> >>
> >>if (nBufferedTuples == 0)
> >>{
> >>/*
> >> * Reset the per-tuple exprcontext. We can only do this
> >> if the
> >> * tuple buffer is empty. (Calling the context the
> >> per-tuple
> >> * memory context is a bit of a misnomer now.)
> >> */
> >>ResetPerTupleExprContext(estate);
> >>}
> >>
> >> but the quals are evaluated in the ExprContext's:
> >>
> >> ExecQual(ExprState *state, ExprContext *econtext)
> >> ...
> >>ret = ExecEvalExprSwitchContext(state, econtext, );
> >>
> >>
> >> which is created with:
> >>
> >> /* Get an EState's per-output-tuple exprcontext, making it if first use */
> >> #define GetPerTupleExprContext(estate) \
> >>((estate)->es_per_tuple_exprcontext ? \
> >> (estate)->es_per_tuple_exprcontext : \
> >> MakePerTupleExprContext(estate))
> >>
> >> and creates its own context:
> >>/*
> >> * Create working memory for expression evaluation in this context.
> >> */
> >>econtext->ecxt_per_tuple_memory =
> >>AllocSetContextCreate(estate->es_query_cxt,
> >> "ExprContext",
> >>
> >> ALLOCSET_DEFAULT_SIZES);
> >>
> >> so this is currently just never reset.
> >
> > Actually, no. The ResetPerTupleExprContext boils down to
> >
> > MemoryContextReset((econtext)->ecxt_per_tuple_memory)
> >
> > and ExecEvalExprSwitchContext does this
> >
> > MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> >
> > So it's resetting the right context, although only on batch boundary.
> >> Seems just using ExecQualAndReset() ought to be sufficient?
> >>
> >
> > That may still be the right thing to do.
> >
>
> Actually, no, because that would reset the context far too early (and
> it's easy to trigger segfaults). So the reset would have to happen after
> processing the row, not this early.
Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting
up the pluggable storage patch into individual pieces...
> But I think the current behavior is actually OK, as it matches what we
> do for defexprs. And the comment before ResetPerTupleExprContext says this:
>
> /*
> * Reset the per-tuple exprcontext. We can only do this if the
> * tuple buffer is empty. (Calling the context the per-tuple
> * memory context is a bit of a misnomer now.)
> */
>
> So the per-tuple context is not quite per-tuple anyway. Sure, we might
> rework that but I don't think that's an issue in this patch.
I'm *not* convinced by this. I think it's bad enough that we do this for
normal COPY, but for WHEN, we could end up *never* resetting before the
end. Consider a case where a single tuple is
Re: House style for DocBook documentation?
On 2019-Jan-21, Joshua D. Drake wrote: > On 1/21/19 10:01 AM, Alvaro Herrera wrote: > > On 2019-Jan-21, Chapman Flack wrote: > > > > > But the point's well taken that in /printed output/, that's of no use. > > > Which is, in a sense, an inconsistency: in one format, you can follow the > > > links, while in another, you're out of luck. > > > > > > Maybe a simpler transform for printed output, rather than collecting > > > all URLs into one section at the back, would just be to follow any > > > that has link text with a containing the same ulink > > > without the link text, so it shows the URL, and that would be right at > > > the bottom of the same 'page'. > > Of course, the text would also be clickable, right? I think putting the > > URL in a footnote is good in that case; it works both on screen and on > > paper, which should alleviate JD's concerns. > > Yeah I could see that. I thought about that but was wondering if it was > possible to auto cite? Sorry, what do you mean with auto cite? Put all links at the end of the section/chapter/book? That seems less usable to me (you have to find out the page where the links appear, and make sure to print the right pages) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: House style for DocBook documentation?
On 1/21/19 10:11 AM, Chapman Flack wrote: On 01/21/19 12:07, Joshua D. Drake wrote: Who is really going to "print" our docs? If they do print the docs, they are likely not going to "type in" a long URL. QR code in footnote (ducks and runs). Funny, although I know why you said that. I don't think it is that bad of an idea. Everyone uses QR Codes now. Have a QR code that automatically opens a page that lists all the expanded links based on the page it is placed? That is pretty cool but I am not sure if that is something that would happen in this round. JD -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Re: House style for DocBook documentation?
On 1/21/19 10:01 AM, Alvaro Herrera wrote: On 2019-Jan-21, Chapman Flack wrote: But the point's well taken that in /printed output/, that's of no use. Which is, in a sense, an inconsistency: in one format, you can follow the links, while in another, you're out of luck. Maybe a simpler transform for printed output, rather than collecting all URLs into one section at the back, would just be to follow any that has link text with a containing the same ulink without the link text, so it shows the URL, and that would be right at the bottom of the same 'page'. Of course, the text would also be clickable, right? I think putting the URL in a footnote is good in that case; it works both on screen and on paper, which should alleviate JD's concerns. Yeah I could see that. I thought about that but was wondering if it was possible to auto cite? JD I wouldn't think it important to apply the same treatment when making HTML. Right, only PDF. -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Re: House style for DocBook documentation?
On 01/21/19 12:07, Joshua D. Drake wrote: > Who is really going to "print" our docs? If they do print the > docs, they are likely not going to "type in" a long URL. QR code in footnote (ducks and runs).
Re: House style for DocBook documentation?
On 2019-Jan-21, Chapman Flack wrote: > But the point's well taken that in /printed output/, that's of no use. > Which is, in a sense, an inconsistency: in one format, you can follow the > links, while in another, you're out of luck. > > Maybe a simpler transform for printed output, rather than collecting > all URLs into one section at the back, would just be to follow any > that has link text with a containing the same ulink > without the link text, so it shows the URL, and that would be right at > the bottom of the same 'page'. Of course, the text would also be clickable, right? I think putting the URL in a footnote is good in that case; it works both on screen and on paper, which should alleviate JD's concerns. > I wouldn't think it important to apply the same treatment when making HTML. Right, only PDF. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Simplify set of flags used by MyXactFlags
On 2019-Jan-18, Michael Paquier wrote: > Keeping both messages makes the error handling at PREPARE time perhaps > a bit cleaner to make the difference about schema-level access or > table-level access, still I'd rather simplify the code and just only > keep the schema-level change as something we complain about. Another > thing is that ACCESSEDTEMPREL is used in PreCommit_on_commit_actions() > to avoid the truncates of ON COMMIT DELETE ROWS if no temporary tables > have been accessed, still it would just mean that truncation would be > tried on empty tables for nothing even if say a function is created in > pg_temp. "... operated on temp namespace" doesn't look good; seems to me to be missing an article, for one thing, but really I'm not sure that 'namespace' is the term to be using here. I'd say "... operated on temporary objects" instead (the plural avoids the need for the article; and the user doesn't really care about the namespace itself but rather about the objects it contains.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Thread-unsafe coding in ecpg
> Thanks for reviewing! I've pushed this now (to HEAD only for the > moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Thanks Tom. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: tickling the lesser contributor's withering ego
On 2018-Dec-27, Magnus Hagander wrote: > On Fri, Dec 21, 2018 at 4:17 PM Alvaro Herrera > wrote: > > > On 2018-Dec-21, Tom Lane wrote: > > > > > Alvaro Herrera writes: > > > > I propose the following patch, which will make those links stable -- > > > > then we can add the following links to the contributors page: > > > > https://www.postgresql/org/docs/10/release-10.html#RELEASE-10-ACKNOWLEDGEMENTS > > > > https://www.postgresql/org/docs/11/release-11.html#RELEASE-11-ACKNOWLEDGEMENTS > > > > > > Seems reasonable, but note the lag time --- unless somebody does > > > something out of the ordinary, those pages won't actually have > > > such tags till after the February minor releases. > > > > Good point. That seems acceptable to me. > > Good. While it *can* be worked around, it's a PITA and it risks getting > overwritten by other things, since the normal docs loads are based off > release tarballs. We can make them off a snapshot tarball, but it's a pain > :) > > Oh, and +1 for stable links like that in general. That would be one good > step. Okay, pushed this. There's no need to do any advance publishing I think; we can wait three more weeks. We still need Erik to come up with the patch for pgweb, though :-) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: jsonpath
On Mon, Jan 21, 2019 at 6:05 PM Oleg Bartunov wrote: > On Mon, Jan 21, 2019 at 1:40 AM Alexander Korotkov > wrote: > > > > On Sun, Jan 20, 2019 at 6:30 AM Alexander Korotkov > > wrote: > > > I'll continue revising this patchset. Nikita, could you please write > > > tests for 3-argument versions of functions? Also, please answer the > > > question regarding "id" property. > > > > I've some more notes regarding function set provided in jsonpath patch: > > 1) Function names don't follow the convention we have. All our > > functions dealing with jsonb have "jsonb_" prefix. Exclusions have > > "jsonb" in other part of name, for example, "to_jsonb(anyelement)". > > We could name functions at SQL level in the same way they are named in > > C. So, they would be jsonb_jsonpath_exists() etc. But it's probably > > too long. What about jsonb_path_exists() etc? > > jsonpath_exists is similar to xpath_exists. That's true. The question is whether it's more important to follow json[b] naming convention or xml/xpath naming convention? I guess json[b] naming convention is more important in our case. > Actually, we could use jsonb_exists(jsonb, jsonpath, json), but then > we should specify the type of the second argument. Yes, but AFAICS the key point of json[b]_ prefixes is to evade function overloading. So, I'm -1 for use jsonb_ prefix and have function overload because of that. > > 2) jsonpath_query_wrapped() name seems counter intuitive for me. What > > about jsonpath_query_array()? > > The question is should we try to provide a functional interface for > all options of > JSON_QUERY clause ? The same is for other SQL/JSON clauses. > Currently, patch contains very limited subset of JSON_QUERY > functionality, mostly for jsonpath testing. Actually, my point is following. We have jsonpath patch close to the committable shape. And we have patch for SQL/JSON clauses including JSON_QUERY, which is huge, complex and didn't receive any serious review yet. So, we'll did our best on that patch during this release cycle, but I can't guarantee it will get in PostgreSQL 12. Thus, my idea is to make jsonpath patch self contained by providing brief and convenient procedural interface. This procedural interface is anyway not a part of standard. It *might* be inspired by standard clauses, but might be not. I think we should try to make this procedural interface as good and convenient by itself. It's our extension, and it wouldn't make us more or less standard conforming. > > 3) jsonpath_query_wrapped() behavior looks tricky to me. It returns > > NULL for no results. When result item is one, it is returned "as is". > > When there are two or more result items, they are wrapped into array. > > This representation of result seems extremely inconvenient for me. It > > becomes hard to solve even trivial tasks with that: for instance, > > iterate all items found. Without extra assumptions on what is going > > to be returned it's also impossible for caller to distinguish single > > array item found from multiple items found wrapped into array. And > > that seems very bad. I know this behavior is inspired by SQL/JSON > > standard. But since these functions are anyway our extension, we can > > implement them as we like. So, I propose to make this function always > > return array of items regardless on count of those items (even for 1 > > or 0 items). > > Fair enough, but if we agree, that we provide an exact functionality of > SQL clauses, then better to follow the standard to avoid problems. No, I see this as our extension. And I don't see problems in being different from standard clauses, because it's different anyway. For me, in this case it's better to evade problems of users. And current behavior of this function seems like just single big pain :) > > 4) If we change behavior of jsonpath_query_wrapped() as above, we can > > also implement jsonpath_query_one() function, which would return first > > result item or NULL for no items. > > Any thoughts? > > I think, that we should postpone this functional interface, which could be > added later. The reason we typically postpone things is that they are hard to bring to committable shape. jsonpath_query_one() doesn't cost us any real development. So, I don't see point in postponing that if consider that as good part of procedural jsonpath interface. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: "ALTER TRIGGER .. RENAME TO" broken with the "Remove WITH OIDS" commit.
Hi, On 2019-01-21 17:02:15 +0530, Rushabh Lathia wrote: > Hi, > > I found that ALTER TRIGGER .. RENAME TO is broken and it's > unable to rename the trigger. Looking further seems renametrig() > function, copy the new trigger name into wrong tuple. This is > broken with below commit: > > commit 578b229718e8f15fa779e20f086c4b6bb3776106 > Author: Andres Freund > Date: Tue Nov 20 15:36:57 2018 -0800 > > Remove WITH OIDS support, change oid catalog column visibility. > > PFA patch to fix the issue. I also added the testcase for the > same into the regression. Thanks for finding and the fix. Pushed!
Re: Thread-unsafe coding in ecpg
"Tsunakawa, Takayuki" writes: > From: Tom Lane [mailto:t...@sss.pgh.pa.us] >> So like this ... > How quick! Thank you. I've reviewed the code for both Unix and Windows, and > it looks OK. I haven't built the patch, but expect the buildfarm to do the > test. Thanks for reviewing! I've pushed this now (to HEAD only for the moment), we'll see what the buildfarm thinks. BTW, I found another spot in descriptor.c where ecpglib is using setlocale() for the same purpose. Perhaps that one's not reachable in threaded apps, but I didn't see any obvious reason to think so, so I changed it too. regards, tom lane
Re: House style for DocBook documentation?
On 1/21/19 8:46 AM, Chapman Flack wrote: On 01/21/19 09:12, Alvaro Herrera wrote: (thinks to self half-seriously about an XSL transform for generating printed output that could preserve link-texted links, add raised numbers, and produce a numbered URLs section at the back) Well, if you have the time and inclination, and you think such changes are improvements, feel free to propose them. Do keep in mind we have a number of outputs that would be good to keep consistent. Well, "consistent" what I was half-thinking about, FSVO "consistent". For me, if I'm looking at an online document, I would prefer to see the descriptive text of the link, rather than a long jaggy URL. If I want to see the URL, I can hover over it, and if I want to go there, I can click it. But the point's well taken that in /printed output/, that's of no use. Which is, in a sense, an inconsistency: in one format, you can follow the links, while in another, you're out of luck. Maybe a simpler transform for printed output, rather than collecting all URLs into one section at the back, would just be to follow any that has link text with a containing the same ulink without the link text, so it shows the URL, and that would be right at the bottom of the same 'page'. That'd be an introductory XSL exercise In practice, applying such a transform "for printed output" would probably mean applying it when generating PDF output, which of course can also be viewed online (and probably most often is, these days). I don't think that is a good idea. PDFs have had the ability to embed hyperlinks under descriptive text for years. If we are going to expand links for printed output, we should have a specific build / modification to the make file for printed output. I also wonder if we are trying to solve the 1% problem here. Who is really going to "print" our docs? If they do print the docs, they are likely not going to "type in" a long URL. They are going to go online (or to the PDF) and click the link that they saw within the printed page. JD -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Re: pgsql: Remove references to Majordomo
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Magnus Hagander writes: > > On Sat, Jan 19, 2019 at 7:19 PM Stephen Frost wrote: > >> Does this also implicitly mean we've just agreed to push back the > >> retirement of the @postgresql.org aliases for the lists until v11 is > >> EOL..? > > > Specifically for pgsql-bugs, yes :) We can special-case that one when the > > time comes, and retire the other ones properly. That might possibly work. > If you're hoping to wait till nobody's copy of Postgres mentions the > @postgresql.org addresses, you're going to be waiting a long time. I was thinking that we would want to make sure that supported versions have the correct address, but if we're fine with special-casing the old aliases and keeping them working, as Magnus suggests, then I suppose it doesn't matter. Thanks! Stephen signature.asc Description: PGP signature
Re: House style for DocBook documentation?
On 01/21/19 09:12, Alvaro Herrera wrote: >> (thinks to self half-seriously about an XSL transform for generating >> printed output that could preserve link-texted links, add raised numbers, >> and produce a numbered URLs section at the back) > > Well, if you have the time and inclination, and you think such changes > are improvements, feel free to propose them. Do keep in mind we have a > number of outputs that would be good to keep consistent. Well, "consistent" what I was half-thinking about, FSVO "consistent". For me, if I'm looking at an online document, I would prefer to see the descriptive text of the link, rather than a long jaggy URL. If I want to see the URL, I can hover over it, and if I want to go there, I can click it. But the point's well taken that in /printed output/, that's of no use. Which is, in a sense, an inconsistency: in one format, you can follow the links, while in another, you're out of luck. Maybe a simpler transform for printed output, rather than collecting all URLs into one section at the back, would just be to follow any that has link text with a containing the same ulink without the link text, so it shows the URL, and that would be right at the bottom of the same 'page'. That'd be an introductory XSL exercise In practice, applying such a transform "for printed output" would probably mean applying it when generating PDF output, which of course can also be viewed online (and probably most often is, these days). So preserving the original ulink run into the text is still of use, in a PDF viewer that can follow links, but adding the footnote ensures that an actual hard copy is still usable. I wouldn't think it important to apply the same treatment when making HTML. So maybe the right value of "consistent" is the one that comes from listing out the various output formats and specifying the right transformation to be applied to each one. (Now wonders if there's a way to do the same transform into HTML while styling the footnote and marker to hide behind @media print) Regards, -Chap
Re: [PROPOSAL] Shared Ispell dictionaries
On 21.01.2019 17:56, Tomas Vondra wrote: On 1/21/19 12:51 PM, Arthur Zakirov wrote: I'll try to implement the syntax, you suggested earlier: ALTER TEXT SEARCH DICTIONARY x UNLOAD/RELOAD The main point here is that UNLOAD/RELOAD can't release the memory immediately, because some other backend may pin a DSM. The second point we should consider (I think) - how do we know which dictionary should be unloaded. There was such function earlier, which was removed. But what about adding an information in the "\dFd" psql's command output? It could be a column which shows is a dictionary loaded. ...The only thing we have is "unload" capability by closing the connection... BTW, even if the connection was closed and there are no other connections a dictionary still remains "loaded". It is because dsm_pin_segment() is called during loading the dictionary into DSM. ... I wonder if we could devise some simple cache eviction policy. We don't have any memory limit GUC anymore, but maybe we could use unload dictionaries that were unused for sufficient amount of time (a couple of minutes or so). Of course, the question is when exactly would it happen (it seems far too expensive to invoke on each dict access, and it should happen even when the dicts are not accessed at all). Yes, I thought about such feature too. Agree, it could be expensive since we need to scan pg_ts_dict table to get list of dictionaries (we can't scan dshash_table). I haven't a good solution yet. I just had a thought to return max_shared_dictionaries_size. Then we can unload dictionaries (and scan the pg_ts_dict table) that were accessed a lot time ago if we reached the size limit. We can't set exact size limit since we can't release the memory immediately. So max_shared_dictionaries_size can be renamed to shared_dictionaries_threshold. If it is equal to "0" then PostgreSQL has unlimited space for dictionaries. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [PATCH] pgbench tap tests fail if the path contains a perl special character
On 1/21/19 11:18 AM, Fabien COELHO wrote: > > Hello Tom, > >> Well, glob does have some metacharacters, so not doing any quoting >> at all still leaves us with failure modes. >> >> I did a little bit of googling on this subject last night, and it >> seems that at least some people believe that the answer is to not >> use glob, period, but read the directory for yourself. > > Yep, would work, it can then be filtered with standard regexpr, > although it will take a few lines (opendir/grep on readdir/closedir). Sure, probably the best solution. Given that Perl has opendir/readdir/closedir, it should be only a handful of lines. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] pgbench tap tests fail if the path contains a perl special character
Hello Tom, Well, glob does have some metacharacters, so not doing any quoting at all still leaves us with failure modes. I did a little bit of googling on this subject last night, and it seems that at least some people believe that the answer is to not use glob, period, but read the directory for yourself. Yep, would work, it can then be filtered with standard regexpr, although it will take a few lines (opendir/grep on readdir/closedir). Maybe there's a better way, but the breakage around backslashes doesn't really leave me hopeful. Indeed. I was thinking of defining my own quote function, but without being convinced that it is such a good idea. As a short-term move to un-break the buildfarm, I'm just going to revert that patch altogether. I was considering to suggest that, so I'm ok with that. We can reapply it once we've figured out how to do the glob part correctly. Yep. No need to spend time much on that, I'll have a look at it, although not right now, allow me a few days. -- Fabien.
Re: [PATCH] pgbench tap tests fail if the path contains a perl special character
Hello Andrew, About windows-specific issues, from File::Glob man page: """ On DOSISH systems, backslash is a valid directory separator character. In this case, use of backslash as a quoting character (via GLOB_QUOTE) interferes with the use of backslash as a directory separator. ... """ It seems to suggest that quotemeta backslash-on-nearly-everything approach is not appropriate. File globbing is not the same thing as regex matching. For example, '.' is a literal character in a glob pattern but a metacharacter in a regex, while ' ' is a literal character in a regex but a globbing metacharacter (but see below), and '*' is a metacharacter in both but has different meanings. quotemeta is intended for regexes but being used here on an expression used for globbing. Perhaps it would be OK we changed back the glob line to use $prefix instead of $qprefix, but kept the use of $qprefix in the later regex. Yep, possibly. I'll have to test, though. To deal with the issue of spaces in file names (not an issue eher ob bowerbird), we should consider adding this: use File::Glob ':bsd_glob'; I was planning that so that the behavior is the same on all systems. This removes the globbing metcharacter nature of the space character, although it might have other effects that cause pain. See `perldoc File::Glob` Yep. Thanks for doing my homework:-) I'll test starting with these options. -- Fabien.
Re: [PATCH] pgbench tap tests fail if the path contains a perl special character
Andrew Dunstan writes: > Perhaps it would be OK we changed back the glob line to use $prefix > instead of $qprefix, but kept the use of $qprefix in the later regex. Well, glob does have some metacharacters, so not doing any quoting at all still leaves us with failure modes. I did a little bit of googling on this subject last night, and it seems that at least some people believe that the answer is to not use glob, period, but read the directory for yourself. Maybe there's a better way, but the breakage around backslashes doesn't really leave me hopeful. As a short-term move to un-break the buildfarm, I'm just going to revert that patch altogether. We can reapply it once we've figured out how to do the glob part correctly. regards, tom lane
Re: [PATCH] pgbench tap tests fail if the path contains a perl special character
On 1/21/19 4:50 AM, Fabien COELHO wrote: > > Hello Tom, > >> Hm, so bowerbird (a Windows machine) has been failing the pgbench tests >> since this went in, cf >> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2019-01-20%2004%3A57%3A01 >> >> >> I'm just guessing, but I suspect that bowerbird is using a path spec >> that >> includes a backslash directory separator and that's somehow bollixing >> things. > > This point is unclear from the log, where plain slashes are used in > the log prefix path, and furthermore no strange characters appear in > the log prefix path: > > # Running: pgbench -n -S -t 50 -c 2 --log > --log-prefix=G:/prog/bf/root/HEAD/pgsql.build/src/bin/pgbench/tmp_check/t_001_pgbench_with_server_main_data/001_pgbench_log_2 > --sampling-rate=0.5 > ok 345 - pgbench logs status (got 0 vs expected 0) > ok 346 - pgbench logs stdout /(?^:select only)/ > ok 347 - pgbench logs stdout /(?^:processed: 100/100)/ > ok 348 - pgbench logs stderr /(?^:^$)/ > not ok 349 - number of log files > >> If so, we might be able to fix it by converting backslashes to >> forward slashes before applying quotemeta(). >> >> It's also possible that on Windows, "glob" handles backslashes >> differently than it does elsewhere, which would be harder to fix >> --- that would bring back my original fear that the appropriate >> quoting rules are different for "glob" than for regexes. > > I'm afraid it could indeed be due to platform-specific behavior, so > testing on the target machine to understand the actual behavior would > help. > > I just tested the current version on my laptop within a directory > containing spaces and other misc chars: Pgbench TAP tests are > currently broken in this context on master, and this may be used to a > collection of issues, not just one, eg pgbench function splits > parameters on spaces which breaks if there are spaces in bdir. > > I'm going to investigate, possibly over next week-end, so please be > patient. > > About windows-specific issues, from File::Glob man page: > > """ On DOSISH systems, backslash is a valid directory separator > character. In this case, use of backslash as a quoting character (via > GLOB_QUOTE) interferes with the use of backslash as a directory > separator. ... """ > > It seems to suggest that quotemeta backslash-on-nearly-everything > approach is not appropriate. > File globbing is not the same thing as regex matching. For example, '.' is a literal character in a glob pattern but a metacharacter in a regex, while ' ' is a literal character in a regex but a globbing metacharacter (but see below), and '*' is a metacharacter in both but has different meanings. quotemeta is intended for regexes but being used here on an expression used for globbing. Perhaps it would be OK we changed back the glob line to use $prefix instead of $qprefix, but kept the use of $qprefix in the later regex. To deal with the issue of spaces in file names (not an issue eher ob bowerbird), we should consider adding this: use File::Glob ':bsd_glob'; This removes the globbing metcharacter nature of the space character, although it might have other effects that cause pain. See `perldoc File::Glob` cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Early WIP/PoC for inlining CTEs
Andreas Karlsson writes: > I have a minor biksheddish question about the syntax. > You proposed: > WITH cte_name [[NOT] MATERIALIZED] AS (query) main_query > While Andrew proposed: > WITH cte_name AS [[NOT] MATERIALIZED] (query) main_query > Do people have any preference between these two? FWIW, I'd independently thought that the latter is more readable, and probably less likely to have syntax problems with future extensions (since AS is already fully reserved). Didn't get around to mentioning it yet, but +1 for putting AS first. regards, tom lane
Re: COPY FROM WHEN condition
On 1/21/19 4:33 AM, Tomas Vondra wrote:
>
>
> On 1/21/19 3:12 AM, Andres Freund wrote:
>> On 2019-01-20 18:08:05 -0800, Andres Freund wrote:
>>> On 2019-01-20 21:00:21 -0500, Tomas Vondra wrote:
On 1/20/19 8:24 PM, Andres Freund wrote:
> Hi,
>
> On 2019-01-20 00:24:05 +0100, Tomas Vondra wrote:
>> On 1/14/19 10:25 PM, Tomas Vondra wrote:
>>> On 12/13/18 8:09 AM, Surafel Temesgen wrote:
On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra
mailto:tomas.von...@2ndquadrant.com>>
wrote:
Can you also update the docs to mention that the functions called
from
the WHERE clause does not see effects of the COPY itself?
/Of course, i also add same comment to insertion method selection
/
>>>
>>> FWIW I've marked this as RFC and plan to get it committed this week.
>>>
>>
>> Pushed, thanks for the patch.
>
> While rebasing the pluggable storage patch ontop of this I noticed that
> the qual appears to be evaluated in query context. Isn't that a bad
> idea? ISMT it should have been evaluated a few lines above, before the:
>
> /* Triggers and stuff need to be invoked in query context. */
> MemoryContextSwitchTo(oldcontext);
>
> Yes, that'd require moving the ExecStoreHeapTuple(), but that seems ok?
>
Yes, I agree. It's a bit too late for me to hack and push stuff, but I'll
fix that tomorrow.
>>>
>>> NP. On second thought, the problem is probably smaller than I thought at
>>> first, because ExecQual() switches to the econtext's per-tuple memory
>>> context. But it's only reset once for each batch, so there's some
>>> wastage. At least worth a comment.
>>
>> I'm tired, but perhaps its actually worse - what's being reset currently
>> is the ESTate's per-tuple context:
>>
>> if (nBufferedTuples == 0)
>> {
>> /*
>> * Reset the per-tuple exprcontext. We can only do this
>> if the
>> * tuple buffer is empty. (Calling the context the
>> per-tuple
>> * memory context is a bit of a misnomer now.)
>> */
>> ResetPerTupleExprContext(estate);
>> }
>>
>> but the quals are evaluated in the ExprContext's:
>>
>> ExecQual(ExprState *state, ExprContext *econtext)
>> ...
>> ret = ExecEvalExprSwitchContext(state, econtext, );
>>
>>
>> which is created with:
>>
>> /* Get an EState's per-output-tuple exprcontext, making it if first use */
>> #define GetPerTupleExprContext(estate) \
>> ((estate)->es_per_tuple_exprcontext ? \
>> (estate)->es_per_tuple_exprcontext : \
>> MakePerTupleExprContext(estate))
>>
>> and creates its own context:
>> /*
>> * Create working memory for expression evaluation in this context.
>> */
>> econtext->ecxt_per_tuple_memory =
>> AllocSetContextCreate(estate->es_query_cxt,
>>"ExprContext",
>>
>> ALLOCSET_DEFAULT_SIZES);
>>
>> so this is currently just never reset.
>
> Actually, no. The ResetPerTupleExprContext boils down to
>
> MemoryContextReset((econtext)->ecxt_per_tuple_memory)
>
> and ExecEvalExprSwitchContext does this
>
> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>
> So it's resetting the right context, although only on batch boundary.
> But now I see 31f38174 does this:
>
> else if (cstate->whereClause != NULL ||
> contain_volatile_functions(cstate->whereClause))
> {
> ...
> insertMethod = CIM_SINGLE;
> }
>
> so it does not do batching with WHERE. But the condition seems wrong, I
> guess it should be && instead of ||. Will investigate in the morning.
>
I think the condition can be just
if (contain_volatile_functions(cstate->whereClause)) { ... }
Per the attached patch. Surafel, do you agree?
>> Seems just using ExecQualAndReset() ought to be sufficient?
>>
>
> That may still be the right thing to do.
>
Actually, no, because that would reset the context far too early (and
it's easy to trigger segfaults). So the reset would have to happen after
processing the row, not this early.
But I think the current behavior is actually OK, as it matches what we
do for defexprs. And the comment before ResetPerTupleExprContext says this:
/*
* Reset the per-tuple exprcontext. We can only do this if the
* tuple buffer is empty. (Calling the context the per-tuple
* memory context is a bit of a misnomer now.)
*/
So the per-tuple context is not quite per-tuple anyway. Sure, we might
rework that but I don't think that's an issue in this patch.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL
Re: jsonpath
On Mon, Jan 21, 2019 at 1:40 AM Alexander Korotkov wrote: > > On Sun, Jan 20, 2019 at 6:30 AM Alexander Korotkov > wrote: > > I'll continue revising this patchset. Nikita, could you please write > > tests for 3-argument versions of functions? Also, please answer the > > question regarding "id" property. > > I've some more notes regarding function set provided in jsonpath patch: > 1) Function names don't follow the convention we have. All our > functions dealing with jsonb have "jsonb_" prefix. Exclusions have > "jsonb" in other part of name, for example, "to_jsonb(anyelement)". > We could name functions at SQL level in the same way they are named in > C. So, they would be jsonb_jsonpath_exists() etc. But it's probably > too long. What about jsonb_path_exists() etc? jsonpath_exists is similar to xpath_exists. Actually, we could use jsonb_exists(jsonb, jsonpath, json), but then we should specify the type of the second argument. > 2) jsonpath_query_wrapped() name seems counter intuitive for me. What > about jsonpath_query_array()? The question is should we try to provide a functional interface for all options of JSON_QUERY clause ? The same is for other SQL/JSON clauses. Currently, patch contains very limited subset of JSON_QUERY functionality, mostly for jsonpath testing. > 3) jsonpath_query_wrapped() behavior looks tricky to me. It returns > NULL for no results. When result item is one, it is returned "as is". > When there are two or more result items, they are wrapped into array. > This representation of result seems extremely inconvenient for me. It > becomes hard to solve even trivial tasks with that: for instance, > iterate all items found. Without extra assumptions on what is going > to be returned it's also impossible for caller to distinguish single > array item found from multiple items found wrapped into array. And > that seems very bad. I know this behavior is inspired by SQL/JSON > standard. But since these functions are anyway our extension, we can > implement them as we like. So, I propose to make this function always > return array of items regardless on count of those items (even for 1 > or 0 items). Fair enough, but if we agree, that we provide an exact functionality of SQL clauses, then better to follow the standard to avoid problems. > 4) If we change behavior of jsonpath_query_wrapped() as above, we can > also implement jsonpath_query_one() function, which would return first > result item or NULL for no items. > Any thoughts? I think, that we should postpone this functional interface, which could be added later. > > -- > Alexander Korotkov > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company -- Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [PROPOSAL] Shared Ispell dictionaries
On 1/21/19 12:51 PM, Arthur Zakirov wrote: > On 21.01.2019 02:43, Tomas Vondra wrote: >> On 1/20/19 11:21 PM, Andres Freund wrote: >>> On 2019-01-20 23:15:35 +0100, Tomas Vondra wrote: Thanks. I've reviewed v17 today and I haven't discovered any new issues so far. If everything goes fine and no one protests, I plan to get it committed over the next week or so. >>> >>> There doesn't seem to be any docs about what's needed to be able to take >>> advantage of shared dicts, and how to prevent them from permanently >>> taking up a significant share of memory. >>> >> >> Yeah, those are good points. I agree the comments might be clearer, but >> essentially ispell dictionaries are shared and everything else is not. >> >> As for the memory consumption / unloading dicts - I agree that's >> something we need to address. There used to be a way to specify memory >> limit and ability to unload dictionaries explicitly, but both features >> have been ditched. The assumption was that UNLOAD would be introduced >> later, but that does not seem to have happened. > > I'll try to implement the syntax, you suggested earlier: > > ALTER TEXT SEARCH DICTIONARY x UNLOAD/RELOAD > > The main point here is that UNLOAD/RELOAD can't release the memory > immediately, because some other backend may pin a DSM. > > The second point we should consider (I think) - how do we know which > dictionary should be unloaded. There was such function earlier, which > was removed. But what about adding an information in the "\dFd" psql's > command output? It could be a column which shows is a dictionary loaded. > The UNLOAD capability is probably a good start, but it's entirely manual and I wonder if it's putting too much burden on the user. I mean, the user has to realize the dictionaries are using a lot of shared memory, has to decide which to unload, and then has to do UNLOAD on it. That's not quite straightforward, especially if there's no way to determine which dictionaries are currently loaded and how much memory they use :-( Of course, the problem is not exactly new - we don't show dictionaries already loaded into private memory. The only thing we have is "unload" capability by closing the connection. OTOH the memory consumption should be much lower thanks to using shared memory. So I think the patch is an improvement even in this regard. I wonder if we could devise some simple cache eviction policy. We don't have any memory limit GUC anymore, but maybe we could use unload dictionaries that were unused for sufficient amount of time (a couple of minutes or so). Of course, the question is when exactly would it happen (it seems far too expensive to invoke on each dict access, and it should happen even when the dicts are not accessed at all). regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: speeding up planning with partitions
Hi, On 1/21/19 4:01 AM, Amit Langote wrote: Rebased again due to 8d8dcead1295. Passes check-world on 8d8dcead1295. I ran a work load on the series, measuring the time to load the data plus the run-time to complete the work load. LoadRun master3m39s 144s 1778 3m12s36s NP [*]2m20s11s [*] Non partitioned case. Best regards, Jesper
Re: House style for DocBook documentation?
On 2019-Jan-19, Chapman Flack wrote: > I have noticed a couple of things: > > - 'SQL' is often marked up as SQL, but far from always. > > - no such markup is applied to 'JSON' or 'XML' at all, at least > not in func.sgml. I think these inconsistencies just stem from lack of a strong reviewer hand on the topic. Let's change that? > - there is a README.links with this guideline: > > o Do not use text with so the URL appears in printed output > > but a grep -r in doc/src/sgml turns up 112 uses that observe the > guideline, and 147 that supply link text. I think the README.links was written in the SGML times; now that we're in XML it may need to be reconsidered. > (thinks to self half-seriously about an XSL transform for generating > printed output that could preserve link-texted links, add raised numbers, > and produce a numbered URLs section at the back) Well, if you have the time and inclination, and you think such changes are improvements, feel free to propose them. Do keep in mind we have a number of outputs that would be good to keep consistent. Thanks, -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
