Re: [HACKERS] Foreign tables don't enforce the partition constraint
> > Similarly, a partition constraint > > should also be enforced at the foreign server. Probably we should > > update documentation of create foreign table to mention this. > > That is a good idea. > > Here's the patch. I am not able to build documents on my laptop because of recent changes in d63762452434a3a046e8c7d130d5a77c594176e4. So, I was not able to check whether the patch builds or not. But I am hoping it builds well. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company cft_doc_change.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] dropping a partition may cause deadlock
Hi,
I noticed that a deadlock can occur due to the way locking when dropping a
partition proceeds. Steps to reproduce:
1. Attach debugger to two sessions, one of which will do a select on the
partitioned parent and the other will drop one of its partitions.
2. In the first debugging session, set a breakpoint at the start of
expand_inherited_rtentry() which is the first point in a select query's
processing where individual partitions will be locked (the parent will
have already been locked by the rewriter).
3. In the second session, set a breakpoint at the start of
heap_drop_with_catalog(), which is the first point in the drop command's
processing where the parent will be locked (the partition will have
already been locked by RangeVarGetRelidExtended()). This will wait for
the first session to release the lock on the parent.
4. In the first session, proceeding with locking of the partition will
cause it wait for the second session that is holding a lock on it; a
deadlock is detected, because that session is waiting for us to release
the lock on the parent.
Attached is a patch to fix that. In the original partitioning patch, I
had aped the approach of index_drop() where the parent heap relation is
locked along with the index relation so that the parent's cached list of
indexes can be invalidated. But I failed to also ape what
RangeVarCallbackForDropRelation() does when dropping an index, which is to
lock the parent heap relation before locking the index relation at all.
For dropping a partition case, it means we lock the parent before we lock
the partition relation.
Will add this to open items list.
Thanks,
Amit
>From acd49444d49835da01aeb31f74872a081e608c53 Mon Sep 17 00:00:00 2001
From: amit
Date: Mon, 3 Apr 2017 15:37:00 +0900
Subject: [PATCH] Fix possibility of deadlock when dropping partitions
The partition relation is locked much earlier than
heap_drop_with_catalog, which is the first point in the drop command's
processing where the parent relation is locked. That reverses the
order in which the locking should happen, which is to lock the parent
before any of its partitions.
---
src/backend/catalog/heap.c | 30 +-
src/backend/commands/tablecmds.c | 30 ++
2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 1cbe7f907f..2dc533ba43 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1758,29 +1758,33 @@ void
heap_drop_with_catalog(Oid relid)
{
Relation rel;
+ HeapTuple tuple;
Oid parentOid;
Relation parent = NULL;
/*
- * Open and lock the relation.
+ * To drop a partition safely, we must grab exclusive lock on its parent,
+ * because another backend might be about to execute a query on the parent
+ * table. If it relies on previously cached partition descriptor, then
+ * it could attempt to access the just-dropped relation as its partition.
+ * We must therefore take a table lock strong enough to prevent all
+ * queries on the table from proceeding until we commit and send out a
+ * shared-cache-inval notice that will make them update their index lists.
*/
- rel = relation_open(relid, AccessExclusiveLock);
-
- /*
- * If the relation is a partition, we must grab exclusive lock on its
- * parent because we need to update its partition descriptor. We must take
- * a table lock strong enough to prevent all queries on the parent from
- * proceeding until we commit and send out a shared-cache-inval notice
- * that will make them update their partition descriptor. Sometimes, doing
- * this is cycles spent uselessly, especially if the parent will be
- * dropped as part of the same command anyway.
- */
- if (rel->rd_rel->relispartition)
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ if (((Form_pg_class) GETSTRUCT(tuple))->relispartition)
{
parentOid = get_partition_parent(relid);
parent = heap_open(parentOid, AccessExclusiveLock);
}
+ ReleaseSysCache(tuple);
+
+ /*
+ * Open and lock the relation.
+ */
+ rel = relation_open(relid, AccessExclusiveLock);
+
/*
* There can no longer be anyone *else* touching the relation, but we
* might still have open queries or cursors, or pending trigger events, in
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d418d56b54..622a572580 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -271,6 +271,7 @@ struct DropRelationCallbackState
{
char relkind;
Oid heapOid;
+ Oid partParentOid;
bool concurrent;
};
@@ -1041,6 +1042,7 @@ RemoveRelations(DropStmt *drop)
/* Look up the appropriate relation using namespace search. */
state.relkind = relkind;
state.heapOid = InvalidOid;
+ state.partParentOid = InvalidOid;
state.concurrent = drop->concurrent;
relOid = RangeVarGetRelidExtended(rel, lockmode, true,
false,
@@ -1070,6 +1072,8
[HACKERS] Unable to build doc on latest head
Hi, Recently my doc build has started failing with errors runtime error: file stylesheet.xsl line 57 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 57 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 57 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 57 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 57 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. runtime error: file stylesheet.xsl line 46 element call-template The called template 'id.attribute' was not found. no result for postgres.xml id.attribute was added by d6376245. [1] says that id.attribute is supported in stylesheets version 1.77.1. Do I need to update stylesheets version? How do I do it? Any help will be appreciated. [1] https://lists.oasis-open.org/archives/docbook-apps/201301/msg00040.html -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] postgres_fdw bug in 9.6
On 2017/04/01 1:32, Jeff Janes wrote: On Thu, Mar 23, 2017 at 5:20 AM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: Done. Attached is a new version of the patch. Is the fix for 9.6.3 going to be just a back port of this, or will it look different? +1 for backporting; although that requires that GetForeignJoinPaths be updated so that the FDW uses a new function to create an alternative local join path (ie, CreateLocalJoinPath), that would make maintenance of the code easy. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] UPDATE of partition key
For some reason, my reply got sent to only Amit Langote instead of
reply-to-all. Below is the mail reply. Thanks Amit Langote for
bringing this to my notice.
On 31 March 2017 at 16:54, Amit Khandekar wrote:
> On 31 March 2017 at 14:04, Amit Langote wrote:
>> On 2017/03/28 19:12, Amit Khandekar wrote:
>>> In the section 5.11 "Partitioning" => subsection 5 "Caveats", I have
>>> removed the caveat of not being able to update partition key. And it
>>> is now replaced by the caveat where an update/delete operations can
>>> silently miss a row when there is a concurrent UPDATE of partition-key
>>> happening.
>>
>> Hmm, how about just removing the "partition-changing updates are
>> disallowed" caveat from the list on the 5.11 Partitioning page and explain
>> the concurrency-related caveats on the UPDATE reference page?
>
> IMHO this caveat is better placed in Partitioning chapter to emphasize
> that it is a drawback specifically in presence of partitioning.
>
>> +If an UPDATE on a partitioned table causes a row to
>> +move to another partition, it is possible that all row-level
>> +BEFORE UPDATE triggers and all row-level
>> +BEFORE DELETE/INSERT
>> +triggers are applied on the respective partitions in a way that is
>> apparent
>> +from the final state of the updated row.
>>
>> How about dropping "it is possible that" from this sentence?
>
> What the statement means is : "It is true that all triggers are
> applied on the respective partitions; but it is possible that they are
> applied in a way that is apparent from final state of the updated
> row". So "possible" applies to "in a way that is apparent..". It
> means, the user should be aware that all the triggers can change the
> row and so the final row will be affected by all those triggers.
> Actually, we have a similar statement for UPSERT involved with
> triggers in the preceding section. I have taken the statement from
> there.
>
>>
>> +UPDATE is done by doing a DELETE
>> on
>>
>> How about: s/is done/is performed/g
>
> Done.
>
>>
>> +triggers are not applied because the UPDATE is
>> converted
>> +to a DELETE and UPDATE.
>>
>> I think you meant DELETE and INSERT.
>
> Oops. Corrected.
>
>>
>> +if (resultRelInfo->ri_PartitionCheck &&
>> +!ExecPartitionCheck(resultRelInfo, slot, estate))
>> +{
>>
>> How about a one-line comment what this block of code does?
>
> Yes, this was needed. Added a comment.
>
>>
>> - * Check the constraints of the tuple. Note that we pass the same
>> + * Check the constraints of the tuple. Note that we pass the same
>>
>> I think that this hunk is not necessary. (I've heard that two spaces
>> after a sentence-ending period is not a problem [1].)
>
> Actually I accidentally removed one space, thinking that it was one of
> my own comments. Reverted back this change, since it is a needless
> hunk.
>
>>
>> + * We have already run partition constraints above, so skip them
>> below.
>>
>> How about: s/run/checked the/g?
>
> Done.
>
>> @@ -2159,6 +2289,7 @@ ExecEndModifyTable(ModifyTableState *node)
>> heap_close(pd->reldesc, NoLock);
>> ExecDropSingleTupleTableSlot(pd->tupslot);
>> }
>> +
>> for (i = 0; i < node->mt_num_partitions; i++)
>> {
>> ResultRelInfo *resultRelInfo = node->mt_partitions + i;
>>
>> Needless hunk.
>
> Right. Removed.
>
>>
>> Overall, I think the patch looks good now. Thanks again for working on it.
>
> Thanks Amit for your efforts in reviewing the patch. Attached is v6
> patch that contains above points handled.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
update-partition-key_v6.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Comment typo in logical/worker.c
Hi, Attached fixes a comment typo in logical/worker.c file. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_typo_logical_worker_c.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] make check-world output
On Sat, Apr 1, 2017 at 4:28 PM, Noah Misch wrote: > The pg_upgrade test suite originated in an age when "make check-world" was > forbidden to depend on Perl; the choice was a shell script or a C program. We > do maintain vcregress.pl:upgradecheck(), a Windows-specific Perl port of the > suite. Maintaining both versions has been tedious. I'd welcome a > high-quality rewrite using src/test/perl facilities. I need to check in my archives, but I recall having a WIP patch doing that: - remove the shell scripts in src/bin/pg_upgrade. - remove upgradecheck in vcregress.pl. - add a TAP test in src/bin/pg_upgrade to run the upgrade tests. So you are interested by that? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REINDEX CONCURRENTLY 2.0
On Fri, Mar 31, 2017 at 5:12 PM, Andreas Karlsson wrote: > Thanks for the feedback. I will look at it when I get the time. > > On 03/31/2017 08:27 AM, Michael Paquier wrote: >> >> - Do a per-index rebuild and not a per-relation rebuild for concurrent >> indexing. Doing a per-relation reindex has the disadvantage that many >> objects need to be created at the same time, and in the case of >> REINDEX CONCURRENTLY time of the operation is not what matters, it is >> how intrusive the operation is. Relations with many indexes would also >> result in much object locks taken at each step. > > > I am personally worried about the amount time spent waiting for long running > transactions if you reindex per index rather than per relation. Because when > you for one index wait on long running transactions nothing prevents new > long transaction from starting, which we will have to wait for while > reindexing the next index. If your database has many long running > transactions more time will be spent waiting than the time spent working. Yup, I am not saying that one approach or the other are bad, both are worth considering. That's a deal between waiting and manual potential cleanup in the event of a failure. > and doing the REINDEX per relation allows for flexibility > since people can still explicitly reindex per index of they want to. You have a point here. I am marking this patch as returned with feedback, this won't get in PG10. If I am freed from the SCRAM-related open items I'll try to give another shot at implementing this feature before the first CF of PG11. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Allow interrupts on waiting standby
On Fri, Mar 31, 2017 at 4:46 PM, Tsunakawa, Takayuki
wrote:
> Thank you, but did you remove WL_LATCH_SET from WaitLatch() intentionally? I
> understood you added it for startup process to respond quickly to events
> other than the postmaster death. Why don't we restore WL_LATCH_SET? I won't
> object to not adding the flag if there's a reason.
It doesn't matter. MyLatch is never set in the context of the startup
process. The other code paths of the startup process using WaitLatch()
may be awaken by the latch set by the WAL receiver, but that does not
apply here.
> I'll mark this as ready for committer when I see WL_LATCH_SET added (optional)
Objection on this point.
> and you have reported that you did the following test cases:
> * Startup process vanishes immediately after postmaster dies, while it is
> waiting for a recovery conflict to be resolved.
> * Startup process vanishes immediately after "pg_ctl stop -m fast", while it
> is waiting for a recovery conflict to be resolved.
> * Startup process resumes WAL application when max_standby_{archive |
> streaming}_delay is changed from the default -1 to a short period, e.g. 10s,
> and "pg_ctl reload" is performed, while it is waiting for a recovery conflict
> to be resolved.
Fine for me, I have checked those multiple scenarios and the startup
process is more responsive. I have emulated the conflict with a
transaction doing repeatable read on the standby while the master was
deleting and vacuuming a table.
But, actually, after looking again at this patch with fresher eyes, I
am being a bit doubtful that this is really correct... Calling
HandleStartupProcInterrupts() is actually dangerous I think, because
it would reload the GUC context while replaying a record. But I think
that it is cleaner to reload the context after being done with a
record.
It seems to me that the correct way to do things would be to switch
all the conflict code paths to use a latch instead of pg_usleep, by
either use a dedicated latch like the recovery wakeup one, or the
MyLatch of the startup process. Then, when a signal arrives in, we
simply set up the conflict latch which wakes up what is waiting for
activity. The latch needs to be reset because calling WaitLatch().
Leaving immediately on WL_POSTMASTER_DEATH looks fine though as far as
I have tested.
As time is growing short, I am marking this patch as returned with
feedback. Thanks for the input, Tsunakawa-san!
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Logical decoding on standby
On 31 March 2017 at 12:49, Craig Ringer wrote: > On 31 March 2017 at 01:16, Andres Freund wrote: >>> @@ -9633,6 +9643,12 @@ xlog_redo(XLogReaderState *record) >>> SetTransactionIdLimit(checkPoint.oldestXid, >>> checkPoint.oldestXidDB); >>> >>> /* >>> + * There can be no concurrent writers to oldestCatalogXmin >>> during >>> + * recovery, so no need to take ProcArrayLock. >>> + */ >>> + ShmemVariableCache->oldestCatalogXmin = >>> checkPoint.oldestCatalogXmin; >> >> s/writers/writes/? > > I meant writers, i.e. nothing else can be writing to it. But writes works too. > Fixed. >>> @@ -276,8 +279,21 @@ CreateInitDecodingContext(char *plugin, >>> >>> ReplicationSlotsComputeRequiredXmin(true); >>> >>> + /* >>> + * If this is the first slot created on the master we won't have a >>> + * persistent record of the oldest safe xid for historic snapshots >>> yet. >>> + * Force one to be recorded so that when we go to replay from this >>> slot we >>> + * know it's safe. >>> + */ >>> + force_standby_snapshot = >>> + !TransactionIdIsValid(ShmemVariableCache->oldestCatalogXmin); >> >> s/first slot/first logical slot/. Also, the reference to master doesn't >> seem right. > > Unsure what you mean re reference to master not seeming right. > > If oldestCatalogXmin is 0 we'll ERROR when trying to start decoding > from the new slot so we need to make sure it gets advanced one we've > decided on our starting catalog_xmin. Moved to next patch, will address there. >>> LWLockRelease(ProcArrayLock); >>> >>> + /* Update ShmemVariableCache->oldestCatalogXmin */ >>> + if (force_standby_snapshot) >>> + LogStandbySnapshot(); >> >> The comment and code don't quite square to me - it's far from obvious >> that LogStandbySnapshot does something like that. I'd even say it's a >> bad idea to have it do that. > > So you prefer the prior approach with separate xl_catalog_xmin advance > records? > > I don't have much preference; I liked the small code reduction of > Simon's proposed approach, but it landed up being a bit awkward in > terms of ordering and locking. I don't think catalog_xmin tracking is > really closely related to the standby snapshot stuff and it feels a > bit like it's a tacked-on afterthought where it is now. This code moved to next patch. But we do need to agree on the best approach. If we're not going to force a standby snapshot here, then it's probably better to use the separate xl_catalog_xmin design instead. >>> /* >>>* tell the snapshot builder to only assemble snapshot once reaching >>> the >>>* running_xact's record with the respective xmin. >>> @@ -376,6 +392,8 @@ CreateDecodingContext(XLogRecPtr start_lsn, >>> start_lsn = slot->data.confirmed_flush; >>> } >>> >>> + EnsureActiveLogicalSlotValid(); >> >> This seems like it should be in a separate patch, and seperately >> reviewed. It's code that's currently unreachable (and thus untestable). > > It is reached and is run, those checks run whenever creating a > non-initial decoding context on master or replica. Again, moved to next patch. >>> /* >>> + * Update our knowledge of the oldest xid we can safely create >>> historic >>> + * snapshots for. >>> + * >>> + * There can be no concurrent writers to oldestCatalogXmin during >>> + * recovery, so no need to take ProcArrayLock. >> >> By now I think is pretty flawed logic, because there can be concurrent >> readers, that need to be safe against oldestCatalogXmin advancing >> concurrently. > > You're right, we'll need a lock or suitable barriers here to ensure > that slot conflict with recovery and startup of new decoding sessions > doesn't see outdated values. This would be the peer of the > pg_memory_barrier() above. Or could just take a lock; there's enough > other locking activity in redo that it should be fine. Now takes ProcArrayLock briefly. oldestCatalogXmin is also used in GetOldestSafeDecodingTransactionId, and there we want to prevent it from being advanced. But on further thought, relying on oldestCatalogXmin there is actually unsafe; on the master, we might've already logged our intent to advance it to some greater value of procArray->replication_slot_catalog_xmin and will do so as ProcArrayLock is released. On standby we're also better off relying on procArray->replication_slot_catalog_xmin since that's what we'll be sending in feedback. Went back to using replication_slot_catalog_xmin here, with comment * * We don't use ShmemVariableCache->oldestCatalogXmin here because another * backend may have already logged its intention to advance it to a higher * value (still <= replication_slot_catalog_xmin) and just be waiting on * ProcArrayLock to actually apply the change. On a standby * replication_slot_catalog_xmin is what the walreceive
Re: [HACKERS] [COMMITTERS] pgsql: Avoid GatherMerge crash when there are no workers.
On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas wrote: > On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund > wrote: > > Hi, > > > > On 2017-04-01 01:22:14 +, Robert Haas wrote: > >> Avoid GatherMerge crash when there are no workers. > > > > I think the gather merge code needs a bit more test coverage (sorry to > > make this a larger theme today). As shown by > > https://coverage.postgresql.org/src/backend/executor/ > nodeGatherMerge.c.gcov.html > > we don't actually merge anything (heap_compare_slots is not exercised). > > Sounds reasonable. > I am working on this and will submit the patch soon. > > > I btw also wonder if it's good that we have a nearly identical copy of > > heap_compare_slots and a bunch of the calling code in both > > nodeMergeAppend.c and nodeGatherMerge.c. On the other hand, it's not > > heavily envolving code. > > Yeah, I don't know. We could alternatively try to move that to some > common location and merge the two implementations. I'm not sure > exactly where, though. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Rushabh Lathia
Re: [HACKERS] Supporting huge pages on Windows
From: Michael Paquier [mailto:michael.paqu...@gmail.com] > Amit is right, you had better use %zu as we do in all the other places of > the code dealing with Size variables that are printed. When compiling with > Visual Studio, Postgres falls back to the implementation of sprintf in > src/port/snprintf.c so that's not something to worry about. Thanks, fixed. Regards Takayuki Tsunakawa win_large_pages_v12.patch Description: win_large_pages_v12.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] parallel explain analyze support not exercised
On Sat, Apr 1, 2017 at 12:25 AM, Andres Freund wrote: > Hi, > > As visible in [1], the explain analyze codepaths of parallel query isn't > exercised in the tests. That used to be not entirely trivial if the > output was to be displayed (due to timing), but we should be able to do > that now that we have the SUMMARY option. > > E.g. > SET max_parallel_workers = 0; > EXPLAIN (analyze, timing off, summary off, costs off) SELECT * FROM blarg2 > WHERE generate_series < 0; > ┌───┐ > │QUERY PLAN │ > ├───┤ > │ Gather (actual rows=0 loops=1)│ > │ Workers Planned: 10 │ > │ Workers Launched: 0 │ > │ -> Parallel Seq Scan on blarg2 (actual rows=0 loops=1) │ > │ Filter: (generate_series < 0) │ > │ Rows Removed by Filter: 1000 │ > └───┘ > > should be reproducible. I'd suggest additionally adding one tests that > throws the EXPLAIN output away, but actually enables paralellism. > > Greetings, > > Andres Freund > > [1] > https://coverage.postgresql.org/src/backend/executor/execParallel.c.gcov.html Please find the attached for the same. -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/ code_coverage.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Documentation improvements for partitioning
On 2017/04/01 6:37, Robert Haas wrote:
> On Tue, Mar 28, 2017 at 6:35 AM, Amit Langote
> wrote:
>> Attached updated patch.
>
> Committed with editing here and there. I just left out the thing
> about UNION ALL views, which seemed to brief a treatment to deserve
> its own subsection. Maybe a longer explanation of that is worthwhile
> or maybe it's not, but that can be a separate patch.
Thanks for committing.
I noticed what looks like a redundant line (or an incomplete sentence) in
the paragraph about multi-column partition keys. In the following text:
+ partitioning, if desired. Of course, this will often result in a
larger
+ number of partitions, each of which is individually smaller.
+ criteria. Using fewer columns may lead to coarser-grained
+ A query accessing the partitioned table will have
+ to scan fewer partitions if the conditions involve some or all of
these
This:
+ criteria. Using fewer columns may lead to coarser-grained
looks redundant. But maybe we can keep that sentence by completing it,
which the attached patch does as follows:
+ number of partitions, each of which is individually smaller. On the
+ other hand, using fewer columns may lead to a coarser-grained
+ partitioning criteria with smaller number of partitions.
The patch also fixes some typos I noticed.
Thanks,
Amit
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 5109778196..340c961b3f 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2932,7 +2932,7 @@ VALUES ('Albany', NULL, NULL, 'NY');
tables and partitions. For example, a partition cannot have any parents
other than the partitioned table it is a partition of, nor can a regular
table inherit from a partitioned table making the latter its parent.
-That means partitioned table and partitions do not participate in
+That means partitioned tables and partitions do not participate in
inheritance with regular tables. Since a partition hierarchy consisting
of the partitioned table and its partitions is still an inheritance
hierarchy, all the normal rules of inheritance apply as described in
@@ -3036,11 +3036,12 @@ CREATE TABLE measurement (
You may decide to use multiple columns in the partition key for range
partitioning, if desired. Of course, this will often result in a larger
- number of partitions, each of which is individually smaller.
- criteria. Using fewer columns may lead to coarser-grained
- A query accessing the partitioned table will have
- to scan fewer partitions if the conditions involve some or all of these
- columns. For example, consider a table range partitioned using columns
+ number of partitions, each of which is individually smaller. On the
+ other hand, using fewer columns may lead to a coarser-grained
+ partitioning criteria with smaller number of partitions. A query
+ accessing the partitioned table will have to scan fewer partitions if
+ the conditions involve some or all of these columns.
+ For example, consider a table range partitioned using columns
lastname and firstname (in that order)
as the partition key.
@@ -3167,8 +3168,8 @@ CREATE INDEX ON measurement_y2008m01 (logdate);
- The simplest option for removing old data is simply to drop the partition
- that is no longer necessary:
+ The simplest option for removing old data is to drop the partition that
+ is no longer necessary:
DROP TABLE measurement_y2006m02;
@@ -3595,8 +3596,8 @@ DO INSTEAD
Partition Maintenance
- To remove old data quickly, simply to drop the partition that is no
- longer necessary:
+ To remove old data quickly, simply drop the partition that is no longer
+ necessary:
DROP TABLE measurement_y2006m02;
@@ -3692,7 +3693,7 @@ ANALYZE measurement;
Triggers or rules will be needed to route rows to the desired
partition, unless the application is explicitly aware of the
partitioning scheme. Triggers may be complicated to write, and will
-be much slower than the tuple routing performed interally by
+be much slower than the tuple routing performed internally by
declarative partitioning.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Supporting huge pages on Windows
On Mon, Apr 3, 2017 at 1:12 PM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-ow...@postgresql.org >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila >> You should use %zu instead of %llu to print Size type of variable. > > I did so at first, but it didn't work with Visual Studio 2010 at hand. It > doesn't support %z which is added in C99. > > But "I" (capital i) instead of "ll" seems appropriate as the following page > says. I chose this. > > https://en.wikipedia.org/wiki/Printf_format_string > > I For signed integer types, causes printf to expect ptrdiff_t-sized integer > argument; for unsigned integer types, causes printf to expect size_t-sized > integer argument. Commonly found in Win32/Win64 platforms. +elog(DEBUG1, "CreateFileMapping(%Iu) with SEC_LARGE_PAGES failed " + "due to insufficient large pages, huge pages disabled", + size); Amit is right, you had better use %zu as we do in all the other places of the code dealing with Size variables that are printed. When compiling with Visual Studio, Postgres falls back to the implementation of sprintf in src/port/snprintf.c so that's not something to worry about. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Alvaro Herrera > Ashutosh Bapat wrote: > > Please add this to the next commitfest. > > If this cannot be reproduced in 9.6, then it must be added to the Open Items > wiki page instead. I added this in next CF. Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Supporting huge pages on Windows
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Amit Kapila > You should use %zu instead of %llu to print Size type of variable. I did so at first, but it didn't work with Visual Studio 2010 at hand. It doesn't support %z which is added in C99. But "I" (capital i) instead of "ll" seems appropriate as the following page says. I chose this. https://en.wikipedia.org/wiki/Printf_format_string I For signed integer types, causes printf to expect ptrdiff_t-sized integer argument; for unsigned integer types, causes printf to expect size_t-sized integer argument. Commonly found in Win32/Win64 platforms. Regards Takayuki Tsunakawa win_large_pages_v11.patch Description: win_large_pages_v11.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On Sat, Apr 1, 2017 at 2:03 AM, David Steele wrote: > Hi, > > On 3/30/17 2:12 PM, Daniel Verite wrote: > >> Vaishnavi Prabakaran wrote: >> >> Hmm, With batch mode, after sending COPY command to server(and server >>> started processing the query and goes into COPY state) , client does not >>> immediately read the result , but it keeps sending other queries to the >>> server. By this time, server already encountered the error >>> scenario(Receiving different message during COPY state) and sent error >>> messages >>> >> >> IOW, the test intentionally violates the protocol and then all goes wonky >> because of that. >> That's why I was wondering upthread what's it's supposed to test. >> I mean, regression tests are meant to warn against a desirable behavior >> being unknowingly changed by new code into an undesirable behavior. >> Here we have the undesirable behavior to start with. >> What kind of regression could we fear from that? >> > Yes, completely agree, demonstrating the undesirable behavior is not needed as documentation gives enough warning to user. The test patch is decided not to go in for now, but will be re-implemented with PSQL commands later. So, during the re-implementation of test patch, I will remove this test. Thanks . > > The CF has been extended until April 7 but time is still growing short. > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this > submission will be marked "Returned with Feedback". > > Thanks for the information, attached the latest patch resolving one compilation warning. And, please discard the test patch as it will be re-implemented later separately. Thanks & Regards, Vaishnavi, Fujitsu Australia. 0001-Pipelining-batch-support-for-libpq-code-v9.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Apr 3, 2017 at 3:50 PM, Thomas Munro wrote: > Please also find attached a rebased patch to add pl/python support, > and new equivalent patches for pl/perl and pl/tcl. I am planning to > add these to PG11 CF1, unless you think we should be more aggressive > given the extra time? Or perhaps the code to inject trigger data transition tables into SPI (a near identical code block these three patches) should be somewhere common so that each PLs would only need to call a function. If so, where should that go? -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Happy to see this committed! And thanks for the co-author credit,
which is a generous exaggeration.
I was still a bit confused about this and poked at it a bit:
On Wed, Mar 8, 2017 at 1:28 PM, Kevin Grittner wrote:
>> /*
>> + * Capture the NEW and OLD transition TABLE tuplestores (if specified for
>> + * this trigger).
>> + */
>> + if (trigdata->tg_newtable || trigdata->tg_oldtable)
>> + {
>> + estate.queryEnv = create_queryEnv();
>> + if (trigdata->tg_newtable)
>> + {
>> + Enr enr = palloc(sizeof(EnrData));
>> +
>> + enr->md.name = trigdata->tg_trigger->tgnewtable;
>> + enr->md.tupdesc = trigdata->tg_relation->rd_att;
>> + enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable);
>> + enr->reldata = trigdata->tg_newtable;
>> + register_enr(estate.queryEnv, enr);
>> + SPI_register_relation(enr);
>> + }
>>
>> Why do we we have to call register_enr and also SPI_register_relation here?
>
> Essentially, because plpgsql does some things through SPI and some
> things not. Both cases are covered.
We're maintaining two different QueryEnvironment objects here, one
inside the SPI module and another in plpgsql_EState. I think that's
done only so that we have one to inject into the portal in
exec_dynquery_with_params, so that EXECUTE 'SELECT * FROM
' can work.
That raises the question: shouldn't SPI_cursor_open just do that
itself using the SPI connection's current QueryEnvironment? That
would make SPI_cursor_open consistent with SPI_execute_plan, and also
benefit handlers for other PLs that would otherwise have to do similar
double-bookkeeping. See attached patch showing what I mean.
Please also find attached a rebased patch to add pl/python support,
and new equivalent patches for pl/perl and pl/tcl. I am planning to
add these to PG11 CF1, unless you think we should be more aggressive
given the extra time?
--
Thomas Munro
http://www.enterprisedb.com
spi-portal-open-with-query-env.patch
Description: Binary data
transition-plpython-v1.patch
Description: Binary data
transition-plperl-v1.patch
Description: Binary data
transition-pltcl-v1.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
> > > That, and I suspect that people will start using this infrastructure > > for some very cool projects. > > Yeah, I was excited to see this committed. It practically begs to be > used for plpgsql TABLE valued variables backed by tuplestores. > (also very excited about this!)
Re: [HACKERS] Partitioned tables and relfilenode
On 2017/04/01 5:29, Robert Haas wrote: > On Fri, Mar 31, 2017 at 4:33 AM, Kyotaro HORIGUCHI > wrote: >> I have no more comment on this. Thank you. > > I committed this with a few tweaks. I simplified the wording in the > documentation a bit, removed or adjusted a couple of comments, and > slightly changed the way the logic works in parseRelOptions in a way > that I think is simpler. Looks good. > Thanks for reviewing, and thanks to Maksim as well, and thanks to Amit > for writing the patch. Thanks for committing. :) Regards, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 1 April 2017 at 02:29, David Fetter wrote: > On Fri, Mar 31, 2017 at 12:20:51PM -0500, Kevin Grittner wrote: >> On Thu, Mar 30, 2017 at 11:51 AM, Kevin Grittner wrote: >> >> > New version attached. It needs some of these problem cases added >> > to the testing, and a mention in the docs that only C and plpgsql >> > triggers can use the feature so far. I'll add those tomorrow. >> >> Done and attached. >> >> Now the question is, should it be pushed? > > Yes. Among other things, that'll get it buildfarm tested and give > people interested in other PLs better visibility. > > That, and I suspect that people will start using this infrastructure > for some very cool projects. Yeah, I was excited to see this committed. It practically begs to be used for plpgsql TABLE valued variables backed by tuplestores. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix obsolete comment in GetSnapshotData
On 31 March 2017 at 21:59, Robert Haas wrote: > On Wed, Mar 29, 2017 at 12:00 AM, Craig Ringer wrote: >> There's an outdated reference to GetOldestXmin(true, true) in >> GetSnapshotData. It hasn't had that call signature for a long while >> now. Update the comment to reflect the current signature. >> >> diff --git a/src/backend/storage/ipc/procarray.c >> b/src/backend/storage/ipc/procarray.c >> index f32881b..4bf0243 100644 >> --- a/src/backend/storage/ipc/procarray.c >> +++ b/src/backend/storage/ipc/procarray.c >> @@ -1556,7 +1556,8 @@ GetMaxSnapshotSubxidCount(void) >> * older than this are known not running any more. >> * RecentGlobalXmin: the global xmin (oldest TransactionXmin across all >> * running transactions, except those running LAZY VACUUM). This >> is >> - * the same computation done by GetOldestXmin(true, true). >> + * the same computation done by >> + * GetOldestXmin(NULL, >> PROCARRAY_FLAGS_DEFAULT|PROCARRAY_FLAGS_VACUUM) >> * RecentGlobalDataXmin: the global xmin for non-catalog tables >> * >= RecentGlobalXmin >> * > > PROCARRAY_FLAGS_VACUUM is defined as a bitwise or with > PROCARRAY_FLAGS_DEFAULT. So or-ing it back with that same value does > not seem quite right. You're right, I muddled it with PROCARRAY_VACUUM_FLAG. PROCARRAY_FLAGS_VACUUM is sufficient. diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index f32881b..4bf0243 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1556,7 +1556,8 @@ GetMaxSnapshotSubxidCount(void) * older than this are known not running any more. * RecentGlobalXmin: the global xmin (oldest TransactionXmin across all * running transactions, except those running LAZY VACUUM). This is - * the same computation done by GetOldestXmin(true, true). + * the same computation done by + * GetOldestXmin(NULL, PROCARRAY_FLAGS_VACUUM) * RecentGlobalDataXmin: the global xmin for non-catalog tables * >= RecentGlobalXmin * -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Fabien COELHO writes: >>> - how desirable/useful is it to have this in 10? >> Extensions and extension-ish packages will love the _NUM vars. > H. I'm afraid pg extension scripts (for CREATE EXTENSION) are not > executed through psql, but server side directly, so there is not much > backslash-command support. Yeah. >> There's a lesser need for the _NAME vars. > I put them more for error reporting, eg: >\if :VERSION_NUM < 11 > \echo :VERSION_NAME is not supported, should be at least 11.0 > \q >\endif I kinda feel like we're getting ahead of ourselves here, in that the above is not going to do what you want until we have some kind of expression eval capability built into psql. You pointed out that "\if false" can be used to reject pre-v10 psqls, but what about rejecting v10? ISTM that if we leave this out until there's something that can do something useful with it, then something along the lines of \if false -- pre-v10, complain and die \endif \if not defined VERSION_NUM -- pre-v11, complain and die \endif would do the trick. Of course, there are probably other ways to do that, but my point is that you haven't made a case why we need to put this in now rather than later. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Covering + unique indexes.
On Thu, Mar 30, 2017 at 8:26 AM, Teodor Sigaev wrote: > Any objection from reviewers to push both patches? I object. Unfortunately, it seems very unlikely that we'll be able to get the patch into shape in the allotted time before feature-freeze, even with the 1 week extension. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Covering + unique indexes.
On Fri, Mar 31, 2017 at 4:31 PM, Peter Geoghegan wrote:
> That's all I have for now. Maybe I can look again later, or tomorrow.
I took another look, this time at code used during CREATE INDEX. More feedback:
* I see no reason to expose _bt_pgaddtup() (to modify it to not be
static, so it can be called during CREATE INDEX for truncated high
key). You could call PageAddItem() directly, just as _bt_pgaddtup()
itself does, and lose nothing. This is the case because the special
steps within _bt_pgaddtup() are only when inserting the first real
item (and only on an internal page). You're only ever using
_bt_pgaddtup() for the high key offset. Would a raw PageAddItem() call
lose anything?
I think I see why you've done this -- the existing CREATE INDEX
_bt_sortaddtup() routine (which is very similar to _bt_pgaddtup(), a
routine used for *insertion*) doesn't do the correct thing were you to
use it, because it assumes that the page is always right most (i.e.,
always has no high key yet).
The reason _bt_sortaddtup() exists is explained here:
* This is almost like nbtinsert.c's _bt_pgaddtup(), but we can't use
* that because it assumes that P_RIGHTMOST() will return the correct
* answer for the page. Here, we don't know yet if the page will be
* rightmost. Offset P_FIRSTKEY is always the first data key.
*/
static void
_bt_sortaddtup(Page page,
Size itemsize,
IndexTuple itup,
OffsetNumber itup_off)
{
...
}
(...thinks some more...)
So, this difference only matters when you have a non-leaf item, which
is never subject to truncation in your patch. So, in fact, it doesn't
matter at all. I guess you should just use _bt_pgaddtup() after all,
rather than bothering with a raw PageAddItem(), even. But, don't
forget to note why this is okay above _bt_sortaddtup().
* Calling PageIndexTupleDelete() within _bt_buildadd(), which
memmove()s all other items on the leaf page, seems wasteful in the
context of CREATE INDEX. Can we do better?
* I also think that calling PageIndexTupleDelete() has a page space
accounting bug, because the following thing happens a second time for
highkey ItemId when new code does this call:
phdr->pd_lower -= sizeof(ItemIdData);
(The first time this happens is within _bt_buildadd() itself, just
before your patch calls PageIndexTupleDelete().)
* I don't think it's okay to let index_truncate_tuple() leak memory
within _bt_buildadd(). It's probably okay for nbtinsert.c callers to
index_truncate_tuple() to not be too careful, though, since those
calls occur in a per-tuple memory context. The same cannot be said for
_bt_buildadd()/CREATE INDEX calls.
* Speaking of memory management: is this really needed?
> @@ -554,7 +580,11 @@ _bt_buildadd(BTWriteState *wstate, BTPageState *state,
> IndexTuple itup)
> * Save a copy of the minimum key for the new page. We have to copy
> * it off the old page, not the new one, in case we are not at leaf
> * level.
> +* Despite oitup is already initialized, it's important to get high
> +* key from the page, since we could have replaced it with truncated
> +* copy. See comment above.
> */
> + oitup = (IndexTuple) PageGetItem(opage,PageGetItemId(opage, P_HIKEY));
> state->btps_minkey = CopyIndexTuple(oitup);
You didn't modify/truncate oitup in-place -- you effectively made a
(truncated) copy by calling index_truncate_tuple(). Maybe you can
manage the memory by assigning keytup to state->btps_minkey, in place
of a CopyIndexTuple(), just for the truncation case?
I haven't studied this in enough detail to be sure that that would be
correct, but it seems clear that a better strategy is needed for
managing memory within _bt_buildadd().
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?
On Mon, Apr 3, 2017 at 7:19 AM, Venkata B Nagothi wrote: > As we are already past the commitfest, I am not sure, what should i change > the patch status to ? The commit fest finishes on the 7th of April. Even with the deadline passed, there is nothing preventing to work on bug fixes. So this item ought to be moved to the next CF with the same category. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: two slab-like memory allocators
Hi, this patch is marked as committed in CF application but the second part (generational allocator) was AFAICS never committed. Does anybody plan to push this forward? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] Bug in Physical Replication Slots (at least 9.5)?
On Fri, Mar 31, 2017 at 4:05 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
> Thank you having a look on this.
>
> # I removed -bugs in CC:.
>
> At Fri, 31 Mar 2017 13:40:00 +1100, Venkata B Nagothi
> wrote in gmail.com>
> > On Fri, Mar 17, 2017 at 6:48 PM, Kyotaro HORIGUCHI <
> > horiguchi.kyot...@lab.ntt.co.jp> wrote:
> >
> > > Hello,
> > >
> > > At Mon, 13 Mar 2017 11:06:00 +1100, Venkata B Nagothi <
> nag1...@gmail.com>
> > > wrote in > > gmail.com>
> > > > On Tue, Jan 17, 2017 at 9:36 PM, Kyotaro HORIGUCHI <
> > > > horiguchi.kyot...@lab.ntt.co.jp> wrote:
> > > > > I managed to reproduce this. A little tweak as the first patch
> > > > > lets the standby to suicide as soon as walreceiver sees a
> > > > > contrecord at the beginning of a segment.
> > > > >
> > > > > - M(aster): createdb as a master with wal_keep_segments = 0
> > > > > (default), min_log_messages = debug2
> > > > > - M: Create a physical repslot.
> > > > > - S(tandby): Setup a standby database.
> > > > > - S: Edit recovery.conf to use the replication slot above then
> > > > > start it.
> > > > > - S: touch /tmp/hoge
> > > > > - M: Run pgbench ...
> > > > > - S: After a while, the standby stops.
> > > > > > LOG: STOP THE SERVER
> > > > >
> > > > > - M: Stop pgbench.
> > > > > - M: Do 'checkpoint;' twice.
> > > > > - S: rm /tmp/hoge
> > > > > - S: Fails to catch up with the following error.
> > > > >
> > > > > > FATAL: could not receive data from WAL stream: ERROR:
> requested
> > > WAL
> > > > > segment 0001002B has already been removed
> > > > >
> > > > >
> > > > I have been testing / reviewing the latest patch
> > > > "0001-Fix-a-bug-of-physical-replication-slot.patch" and i think, i
> might
> > > > need some more clarification on this.
> > > >
> > > > Before applying the patch, I tried re-producing the above error -
> > > >
> > > > - I had master->standby in streaming replication
> > > > - Took the backup of master
> > > >- with a low max_wal_size and wal_keep_segments = 0
> > > > - Configured standby with recovery.conf
> > > > - Created replication slot on master
> > > > - Configured the replication slot on standby and started the standby
> > >
> > > I suppose the "configure" means primary_slot_name in recovery.conf.
> > >
> > > > - I got the below error
> > > >
> > > >>> 2017-03-10 11:58:15.704 AEDT [478] LOG: invalid record length
> at
> > > > 0/F2000140: wanted 24, got 0
> > > >>> 2017-03-10 11:58:15.706 AEDT [481] LOG: started streaming WAL
> from
> > > > primary at 0/F200 on timeline 1
> > > >>> 2017-03-10 11:58:15.706 AEDT [481] FATAL: could not receive
> data
> > > > from WAL stream: ERROR: requested WAL segment
> 000100F2
> > > has
> > > > already been removed
> > >
> > > Maybe you created the master slot with non-reserve (default) mode
> > > and put a some-minites pause after making the backup and before
> > > starting the standby. For the case the master slot doesn't keep
> > > WAL segments unless the standby connects so a couple of
> > > checkpoints can blow away the first segment required by the
> > > standby. This is quite reasonable behavior. The following steps
> > > makes this more sure.
> > >
> > > > - Took the backup of master
> > > >- with a low max_wal_size = 2 and wal_keep_segments = 0
> > > > - Configured standby with recovery.conf
> > > > - Created replication slot on master
> > > + - SELECT pg_switch_wal(); on master twice.
> > > + - checkpoint; on master twice.
> > > > - Configured the replication slot on standby and started the standby
> > >
> > > Creating the slot with the following command will save it.
> > >
> > > =# select pg_create_physical_replication_slot('s1', true);
> > >
> >
> > I did a test again, by applying the patch and I am not sure if the patch
> is
> > doing the right thing ?
>
> This is a bit difficult to make it happen.
>
Yes, it seems to be quite difficult to re-produce.
>
> > Here is test case -
> >
> > - I ran pgbench
> > - I took the backup of the master first
> >
> > - Below are the WALs on master after the stop backup -
> >
> > postgres=# select pg_stop_backup();
> >
> > NOTICE: WAL archiving is not enabled; you must ensure that all required
> > WAL segments are copied through other means to complete the backup
> > pg_stop_backup
> >
> > 0/8C000130
> > (1 row)
> >
> > postgres=# \q
> > [dba@buildhost data]$ ls -ltrh pgdata-10dev-prsb-1/pg_wal/
> > total 65M
> > drwx--. 2 dba dba 4.0K Mar 31 09:36 archive_status
> > -rw---. 1 dba dba 16M Mar 31 11:09 0001008E
> > -rw---. 1 dba dba 16M Mar 31 11:17 0001008F
> > -rw---. 1 dba dba 16M Mar 31 11:18 0001008C
> > -rw---. 1 dba dba 16M Mar 31 11:18 0001008D
> >
> > - After the backup, i created the physical replication slot
> >
> >
> > postgres=# select pg_create_physical_replication_slot('repslot',true);
> >
>
Re: [HACKERS] Undefined psql variables
I'm inclined to suggest that we should require all extensions beyond the boolean-literal case to be set up as a keyword followed by appropriate argument(s); that seems like it's enough to prevent syntax conflicts from future additions. So you could imagine \if defined varname \if sql boolean expression to send to server \if compare value operator value I'm still thinking:-) Independently of the my aethetical complaint against having a pretty unusual keyword prefix syntax, how would you envision a \set assignment variant? Would \if have a different expression syntax somehow? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Undefined psql variables
Hello,
For checking variable definition, I would suggest to extend the variable
access syntax so that there is no exception to the one thing rule between
client side and server side evaluation:
\if :?variable
Don't like that one bit;
Possibly:-)
This is kind of a shell-like hack ${VAR:?error-message-if-not-defined},
or ${#VAR} to get a length.
They are not likable but they do the job.
you're going to run out of namespace there in no time.
I do not undestand where there would be a namespace issue. Is that under
the assumption that ":?xxx" is frequently used in SQL?
And you don't have a very good way to say "if not defined", either.
Indeed. I'm afraid that handling "NOT" client-side would be necessary with
this approach, so the decision would be 1 thing or 2 things where the
first one is "NOT" would be handled client-side.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Other formats in pset like markdown, rst, mediawiki
2017-03-23 17:26 GMT+01:00 Pierre Ducroquet :
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation:tested, passed
>
> Hi
>
> This is my first review (Magnus said in his presentation in PGDay Paris
> that volunteers should just come and help, so here I am), so please notify
> me for any mistake I do when using the review tools...
>
> The feature seems to work as expected, but I don't claim to be a markdown
> and rst expert.
> Some minor issues with the code itself :
> - some indentation issues (documentation and code itself with mix between
> space based and tab based indentation) and a few trailing spaces in code
> - typographic issues in the documentation :
> - "The html, asciidoc, latex, latex-longtable, troff-ms, and markdown
> and rst formats" ==> duplicated and
> - "Sets the output format to one of unaligned, aligned, wrapped, html,
> asciidoc, latex (uses tabular), latex-longtable, rst, markdown, or
> troff-ms." ==> extra comma at the end of the list
> - the comment " dont add line after last row, because line is added after
> every row" is misleading, it should warn that it's only for rst
> - there is a block of commented out code left
> - in the print_aligned_vertical function, there is a mix between
> "cont->opt->format == PRINT_RST" and "format == &pg_rst" and I don't see
> any obvious reason for that
> - the documentation doesn't mention (but ok, it's kind of obvious) that
> the linestyle option will not work with rst and markdown
>
> Thanks !
>
> The new status of this patch is: Waiting on Author
>
Corrected problem with \pset linestyle when format is set to markdown, or
rst.
Corrected tuples only for markdown and rst (normal and expanded)
Jan
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
--
Jelen
Starší čeledín datovýho chlíva
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ad463e71c1..b513aa703b 100644
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
***
*** 2470,2476 lo_import 152801
aligned, wrapped,
html, asciidoc,
latex (uses tabular),
! latex-longtable, or
troff-ms.
Unique abbreviations are allowed. (That would mean one letter
is enough.)
--- 2470,2477
aligned, wrapped,
html, asciidoc,
latex (uses tabular),
! latex-longtable,
! rst, markdown, or
troff-ms.
Unique abbreviations are allowed. (That would mean one letter
is enough.)
***
*** 2498,2504 lo_import 152801
The html, asciidoc, latex,
! latex-longtable, and troff-ms
formats put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! This might not be
--- 2499,2506
The html, asciidoc, latex,
! latex-longtable, troff-ms,
! markdown and rst
formats put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! This might not be
diff --git a/src/bin/psql/command.c bindex 94a3cfce90..9b5c4c9eec 100644
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 3684,3689 _align2string(enum printFormat in)
--- 3684,3695
case PRINT_TROFF_MS:
return "troff-ms";
break;
+ case PRINT_MARKDOWN:
+ return "markdown";
+ break;
+ case PRINT_RST:
+ return "rst";
+ break;
}
return "unknown";
}
***
*** 3755,3763 do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
popt->topt.format = PRINT_LATEX_LONGTABLE;
else if (pg_strncasecmp("troff-ms", value, vallen) == 0)
popt->topt.format = PRINT_TROFF_MS;
else
{
! psql_error("\\pset: allowed formats are unaligned, aligned, wrapped, html, asciidoc, latex, latex-longtable, troff-ms\n");
return false;
}
}
--- 3761,3773
popt->topt.format = PRINT_LATEX_LONGTABLE;
else if (pg_strncasecmp("troff-ms", value, vallen) == 0)
popt->topt.format = PRINT_TROFF_MS;
+ else if (pg_strncasecmp("markdown", value, vallen) == 0)
+ popt->topt.format = PRINT_MARKDOWN;
+ else if (pg_strncasecmp("rst", value, vallen) == 0)
+ popt->topt.format = PRINT_RST;
else
{
! psql_error("\\pset: allowed formats are unaligned, aligned, wrapped, html, asciidoc, latex, latex-longtable, troff-ms, markdown, rst\n");
return false;
}
}
diff --git a/src/bin/psql/helindex ac435220e6..13f2e44add 100644
***
Re: [HACKERS] Undefined psql variables
Hello Tom, I'm just arguing that for pgbench the evaluator needs to be on the client side, which implies a lexer, parser and executor. For psql, it does not really matter where the evaluator is, thus relying on the server should be fine and simpler and also powerful, provided the necessary information can be transfered from the client, eg through variable expansion, and maybe back in the form of special variables to test for errors for instance. I don't really buy this. Certainly it'd be fine for many use-cases, but there will be cases where what you're trying to script around is server-side errors. An expression evaluation facility that goes belly-up as soon as the server is in an aborted transaction is not going to be very useful in that scenario. "Going belly-up" suggests testing/checking for errors, which could be eased through special variables à la errno and more than simplistic client-side expression evaluation. I think that we need just a relatively primitive facility in order to meet that sort of need, but we do need something. Hmmm. Yes, I was thinking of that kind of thing. The question is how large the necessary "something". I'm arguying for the smallest possible solution. Maybe handling direct booleans (as already implemented) and the NOT operator could be enough (clear enough to understand for the user, would cover needed cases, and would be easy to implement)? i.e. \if NOT :IS_CONNECTED ... SELECT ... \gset \if :SQL_ERROR_OCCURED ... \if :CURRENT_TRANSACTION_ABORTED ... So my view of this is that "send the expression to the server" ought to be just one option for \if, not the only way to do it. Hence my suggestion of "\if sql ...text to send to server...". Probably someone can think of a better keyword than "sql" for that. That is the kind of (ugly) thing I would really like to avoid, if possible. As pavel argued, it should be "intuitive", and having a explicit syntactic marker and/or possibly two distinct syntaxes does not strike me as a desirable user-experience. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
"Daniel Verite" writes: > Tom Lane wrote: >> If we do phrase it like that, I think that the most natural behavior >> for \r is the way I have it in HEAD --- you'd expect it to clear >> the query buffer, but you would not expect it to forget the most >> recent command. > Okay, leaving out \r as it is and changing only \p works for me. Sold, I'll adjust your patch and push it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Undefined psql variables
Fabien COELHO writes: > For checking variable definition, I would suggest to extend the variable > access syntax so that there is no exception to the one thing rule between > client side and server side evaluation: >\if :?variable Don't like that one bit; you're going to run out of namespace there in no time. And you don't have a very good way to say "if not defined", either. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
More to the point, we already have that. See c.h:
#define CppAsString(identifier) #identifier
#define CppAsString2(x) CppAsString(x)
Thanks for the pointer. I grepped for them without success. Here is a v4.
--
Fabiendiff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ad463e7..b1508be 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3580,6 +3580,18 @@ bar
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+These variables are set when connected to reflects the server's version
+both in short name (eg "9.6.2", "10.0") and number (eg 90602, 10).
+They can be changed or unset.
+
+
+
+
+
SINGLELINE
@@ -3625,10 +3637,13 @@ bar
VERSION
+VERSION_NAME
+VERSION_NUM
-This variable is set at program start-up to
-reflect psql's version. It can be changed or unset.
+These variables are set at program start-up to reflect
+psql's version in verbose, short name ("10.0")
+and number (10). They can be changed or unset.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..2c58cfb 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3320,6 +3320,8 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ char vbuf[32];
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
@@ -3331,6 +,12 @@ SyncVariables(void)
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ /* server version information */
+ SetVariable(pset.vars, "SERVER_VERSION_NAME",
+formatPGVersionNumber(pset.sversion, true, vbuf, sizeof(vbuf)));
+ snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3349,6 +3357,8 @@ UnsyncVariables(void)
SetVariable(pset.vars, "HOST", NULL);
SetVariable(pset.vars, "PORT", NULL);
SetVariable(pset.vars, "ENCODING", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 8068a28..68b6724 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,8 @@ main(int argc, char *argv[])
EstablishVariableSpace();
SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+ SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+ SetVariable(pset.vars, "VERSION_NUM", CppAsString2(PG_VERSION_NUM));
/* Default values for variables (that don't match the result of \unset) */
SetVariableBool(pset.vars, "AUTOCOMMIT");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Undefined psql variables
Fabien COELHO writes: > I'm just arguing that for pgbench the evaluator needs to be on the client > side, which implies a lexer, parser and executor. For psql, it does not > really matter where the evaluator is, thus relying on the server should be > fine and simpler and also powerful, provided the necessary information can > be transfered from the client, eg through variable expansion, and maybe > back in the form of special variables to test for errors for instance. I don't really buy this. Certainly it'd be fine for many use-cases, but there will be cases where what you're trying to script around is server-side errors. An expression evaluation facility that goes belly-up as soon as the server is in an aborted transaction is not going to be very useful in that scenario. I think that we need just a relatively primitive facility in order to meet that sort of need, but we do need something. So my view of this is that "send the expression to the server" ought to be just one option for \if, not the only way to do it. Hence my suggestion of "\if sql ...text to send to server...". Probably someone can think of a better keyword than "sql" for that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
src/tools/msvc/Solution.pm:s{PG_VERSION_STR "[^"]+"}{__STRINGIFY(x)
#x\n#define __STRINGIFY2(z) __STRINGIFY(z)\n#define PG_VERSION_STR
"PostgreSQL $self->{strver}$extraver, compiled by Visual C++ build "
__STRINGIFY2(_MSC_VER) ", $bits-bit"};
Well, this is the same hack.
Without digging too deep, it seems like the redefinition wouldn't be
harmful, but it might make sense to not use the name STRINGIFY() if only to
avoid confusion with Solution.pm.
It is the usual name for these macro. What would you suggest?
- how desirable/useful is it to have this in 10?
Extensions and extension-ish packages will love the _NUM vars.
H. I'm afraid pg extension scripts (for CREATE EXTENSION) are not
executed through psql, but server side directly, so there is not much
backslash-command support.
There's a lesser need for the _NAME vars.
I put them more for error reporting, eg:
\if :VERSION_NUM < 11
\echo :VERSION_NAME is not supported, should be at least 11.0
\q
\endif
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Undefined psql variables
Hello Pavel,
I have convinced myself that, unlike pgbench, psql does not really need an
advanced client-side-implemented language, so the smaller the better. What
I mean by this is that from psql point of view it is ok that the actual
expression evaluation is performed server-side. From a user experience
point of view it would look similar to pgbench, just the evaluator does not
need to be client-side.
I am sorry - I disagree - I don't expect hard scripting in psql too. But
psql is much more widely used than pgbench - and scripting should be
intuitive.
I am ok with that objective.
I'm just arguing that for pgbench the evaluator needs to be on the client
side, which implies a lexer, parser and executor. For psql, it does not
really matter where the evaluator is, thus relying on the server should be
fine and simpler and also powerful, provided the necessary information can
be transfered from the client, eg through variable expansion, and maybe
back in the form of special variables to test for errors for instance.
It would not change anything from a syntactic point of view, i.e. it
should indeed be intuitive as you put it, i.e. SQL-like, for instance:
\if current_setting('something') = 'whatever' AND :VERSION_NUM >= 10
...
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Corey Huinker writes: > Looking at #define STRINGIFY(), I got curious where else STRINGIFY was used: > Without digging too deep, it seems like the redefinition wouldn't be > harmful, but it might make sense to not use the name STRINGIFY() if only to > avoid confusion with Solution.pm. More to the point, we already have that. See c.h: #define CppAsString(identifier) #identifier #define CppAsString2(x) CppAsString(x) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] walsender.c comments
On Tue, Mar 28, 2017 at 8:59 AM, Erik Rijkers wrote: > Small fry gathered wile reading walsender.c ... > > (to be applied to master) > Thanks, applied. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Suggested fix for \p and \r in psql
Tom Lane wrote: > If we do phrase it like that, I think that the most natural behavior > for \r is the way I have it in HEAD --- you'd expect it to clear > the query buffer, but you would not expect it to forget the most > recent command. Okay, leaving out \r as it is and changing only \p works for me. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
On Sun, Apr 2, 2017 at 12:29 PM, Fabien COELHO wrote:
>
> Hello Corey,
>
> I'm anxious to help with these patches, but they seem a bit of a moving
>> target. Happy to jump in and review as soon as we've settled on what
>> should
>> be done.
>>
>
> The "v3" I sent basically adds both client & server version numbers in
> client-side variables, basically same code as suggested by Pavel for the
> server version, and some documentation.
>
patch applies via patch -p1
Works as advertised.
# \echo SERVER_VERSION_NAME
SERVER_VERSION_NAME
# \echo :SERVER_VERSION_NAME
10.0
# \echo :SERVER_VERSION_NUM
10
# \echo :VERSION_NUM
10
The new documentation is clear, and accurately reflects current name style.
Looking at #define STRINGIFY(), I got curious where else STRINGIFY was used:
$ git grep STRINGIFY
src/bin/psql/startup.c:#define STRINGIFY2(symbol) #symbol
src/bin/psql/startup.c:#define STRINGIFY(symbol) STRINGIFY2(symbol)
src/bin/psql/startup.c: SetVariable(pset.vars, "VERSION_NUM",
STRINGIFY(PG_VERSION_NUM));
src/tools/msvc/Solution.pm:s{PG_VERSION_STR "[^"]+"}{__STRINGIFY(x)
#x\n#define __STRINGIFY2(z) __STRINGIFY(z)\n#define PG_VERSION_STR
"PostgreSQL $self->{strver}$extraver, compiled by Visual C++ build "
__STRINGIFY2(_MSC_VER) ", $bits-bit"};
Without digging too deep, it seems like the redefinition wouldn't be
harmful, but it might make sense to not use the name STRINGIFY() if only to
avoid confusion with Solution.pm.
> The questions are:
>
> - which version should be provided (10.0 10 ...)
>
A fixed length string without decimals seems best for the multitude of
tools that will want to manipulate that data.
> - how should they be named?
>
>In v3 there is VERSION{,_NAME,_NUM} for client and
>SERVER_VERSION_{NUM,NAME} or SVERSION_NUM suggested
>by Pavel for server.
>
SERVER_VERSION_* is good.
VERSION_* is ok. Would CLIENT_VERSION_* or PSQL_VERSION_* be better?
> - how desirable/useful is it to have this in 10?
>
Extensions and extension-ish packages will love the _NUM vars. The sooner
the better.
There's a lesser need for the _NAME vars.
Re: [HACKERS] Undefined psql variables
2017-04-02 18:56 GMT+02:00 Fabien COELHO : > > Hello Tom, > > I'm inclined to suggest that we should require all extensions beyond the >> boolean-literal case to be set up as a keyword followed by appropriate >> argument(s); that seems like it's enough to prevent syntax conflicts from >> future additions. So you could imagine >> >> \if defined varname >> \if sql boolean expression to send to server >> \if compare value operator value >> >> It would be easy to allow "not" in front of any one of these, but >> it's less clear how to do AND or OR combinations. You can always >> fake AND with nested \if's, but OR is a bit more of a problem. >> Maybe we don't need it. >> >> Other ideas about how to design this? >> > > My 0.02 €: > > I have convinced myself that, unlike pgbench, psql does not really need an > advanced client-side-implemented language, so the smaller the better. What > I mean by this is that from psql point of view it is ok that the actual > expression evaluation is performed server-side. From a user experience > point of view it would look similar to pgbench, just the evaluator does not > need to be client-side. > I am sorry - I disagree - I don't expect hard scripting in psql too. But psql is much more widely used than pgbench - and scripting should be intuitive. Regards Pavel > > So I would suggest something close but maybe simpler than what you suggest > above. If there is just one thing, it is true or false, checked client > side, well, this is already implemented:-). > > \if something > > If there are more than one argument, or maybe if previous true/false > evaluation failed, then: > > \if sql expression to be evaluated server side > > Then the result is checked for true or false client-side. It would be > equivalent to: > > SELECT sql expression to be evaluted server side AS is_ok \gset > \if :is_ok > > Finally I would suggest that client to server would only communicate by > variable substitution, as the backtick patch with external processes. > > For checking variable definition, I would suggest to extend the variable > access syntax so that there is no exception to the one thing rule between > client side and server side evaluation: > > \if :?variable > > the :?... is subsituted by true or false depending on whether the variable > exists. > > \if NOT :?variable > > would work by executing "NOT ..." on the server. No need for "defined" > which would not look like SQL function calls anyway, no need for any > operator client side or clumsy rules. > > -- > Fabien. > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Undefined psql variables
2017-04-02 18:29 GMT+02:00 Tom Lane : > Corey Huinker writes: > > On Mon, Jan 23, 2017 at 12:53 PM, Tom Lane wrote: > >> This seems pretty bizarre. What's the use case? Why would it not > >> be better to build the behavior out of other spare parts, along the > >> lines of COALESCE or perhaps > >> \if not defined(x) > > > In light of the backticks variable expansion thread, I'm reviving this > > thread in the hopes that a defined()-ish psql function can make it into > v10. > > It's something that cannot be solved with a query and \gset, so adding it > > to psql boolean expressions is the only option I can see. > > I'm fairly hesitant to add stuff in advance of having a fairly clear > sketch of the boolean expression language we want. I don't mind > implementing such a language piece-by-piece, but if we just throw in > one or two features that seem like good ideas, I'm afraid we'll be > painting ourselves into a corner. > > The only thing that seems locked down so far is that "a single argument > is a simple boolean value". If we were hot to support expr-style > comparison behavior, we could define cases with exactly three arguments > as being "\if value operator value". But I'm afraid that that would > cause problems because there would be other desirable behaviors (like > "\if not defined varname") that would also involve three arguments, > creating ambiguity. > > I'm inclined to suggest that we should require all extensions beyond the > boolean-literal case to be set up as a keyword followed by appropriate > argument(s); that seems like it's enough to prevent syntax conflicts from > future additions. So you could imagine > > \if defined varname > \if sql boolean expression to send to server > \if compare value operator value > These possibilities looks well. if defined varname is perfectly intuitive Maybe it can be shorter - def, undef \if def var, \if undef var Regards Pavel > It would be easy to allow "not" in front of any one of these, but > it's less clear how to do AND or OR combinations. You can always > fake AND with nested \if's, but OR is a bit more of a problem. > Maybe we don't need it. > > Other ideas about how to design this? > > regards, tom lane > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] Undefined psql variables
Hello Tom, I'm inclined to suggest that we should require all extensions beyond the boolean-literal case to be set up as a keyword followed by appropriate argument(s); that seems like it's enough to prevent syntax conflicts from future additions. So you could imagine \if defined varname \if sql boolean expression to send to server \if compare value operator value It would be easy to allow "not" in front of any one of these, but it's less clear how to do AND or OR combinations. You can always fake AND with nested \if's, but OR is a bit more of a problem. Maybe we don't need it. Other ideas about how to design this? My 0.02 €: I have convinced myself that, unlike pgbench, psql does not really need an advanced client-side-implemented language, so the smaller the better. What I mean by this is that from psql point of view it is ok that the actual expression evaluation is performed server-side. From a user experience point of view it would look similar to pgbench, just the evaluator does not need to be client-side. So I would suggest something close but maybe simpler than what you suggest above. If there is just one thing, it is true or false, checked client side, well, this is already implemented:-). \if something If there are more than one argument, or maybe if previous true/false evaluation failed, then: \if sql expression to be evaluated server side Then the result is checked for true or false client-side. It would be equivalent to: SELECT sql expression to be evaluted server side AS is_ok \gset \if :is_ok Finally I would suggest that client to server would only communicate by variable substitution, as the backtick patch with external processes. For checking variable definition, I would suggest to extend the variable access syntax so that there is no exception to the one thing rule between client side and server side evaluation: \if :?variable the :?... is subsituted by true or false depending on whether the variable exists. \if NOT :?variable would work by executing "NOT ..." on the server. No need for "defined" which would not look like SQL function calls anyway, no need for any operator client side or clumsy rules. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
"Daniel Verite" writes: > The documentation over-simplifies things as if there > was only one query buffer, instead of two of them. Yeah, there's a lot of oversimplification in the docs for slash commands --- for instance, I was just noticing yesterday that there's no mention of the variant argument-parsing behavior of slash commands that use OT_WHOLE_LINE or OT_FILEPIPE parsing. It would be good to make some effort to improve that. It seems like a separate question from what the code should do, though. My first thought about how to document the query-buffer behavior is to continue to speak as though there is only one query buffer, but to add, for example, for the \g command "If the query buffer is empty then the most recent command is re-executed". If we do phrase it like that, I think that the most natural behavior for \r is the way I have it in HEAD --- you'd expect it to clear the query buffer, but you would not expect it to forget the most recent command. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
Tom Lane wrote: > > 1. \p ignores the "previous buffer". Example: > > Yeah, I did that intentionally, thinking that the old behavior was > confusing. We can certainly discuss it though. I'd tend to agree > with your point that \p and \w should print the same thing, but > maybe neither of them should look at the previous_buf. > > > 2. \r keeps the "previous buffer". I think it should clear it. > > I don't really agree with this. The fact that it used to clear both > buffers was an implementation accident that probably nobody had even > understood clearly. ISTM that loses functionality because you can't > do \g anymore. I don't have a strong opinion on \r. As a user I tend to use Ctrl+C rather than \r for the edit in progress. The documentation over-simplifies things as if there was only one query buffer, instead of two of them. \r or \reset Resets (clears) the query buffer. From just that reading, the behavior doesn't seem right when we "clear the query buffer", and then print it, and it doesn't come out as empty. About \p alone, if it doesn't output what \g is about to run, I think that's a problem because ISTM that it's the main use case of \p Following the chain of consistency, the starting point seems to be that \g sends the previous query if the edit-in-progress input buffer is empty, instead of saying, empty buffer = empty query, so nothing to send. Presumably the usage of issuing \g alone to repeat the last query is well established, so we can't change it and need to adjust the other commands to be as less surprising as possible with our two buffers. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Undefined psql variables
Corey Huinker writes: > On Mon, Jan 23, 2017 at 12:53 PM, Tom Lane wrote: >> This seems pretty bizarre. What's the use case? Why would it not >> be better to build the behavior out of other spare parts, along the >> lines of COALESCE or perhaps >> \if not defined(x) > In light of the backticks variable expansion thread, I'm reviving this > thread in the hopes that a defined()-ish psql function can make it into v10. > It's something that cannot be solved with a query and \gset, so adding it > to psql boolean expressions is the only option I can see. I'm fairly hesitant to add stuff in advance of having a fairly clear sketch of the boolean expression language we want. I don't mind implementing such a language piece-by-piece, but if we just throw in one or two features that seem like good ideas, I'm afraid we'll be painting ourselves into a corner. The only thing that seems locked down so far is that "a single argument is a simple boolean value". If we were hot to support expr-style comparison behavior, we could define cases with exactly three arguments as being "\if value operator value". But I'm afraid that that would cause problems because there would be other desirable behaviors (like "\if not defined varname") that would also involve three arguments, creating ambiguity. I'm inclined to suggest that we should require all extensions beyond the boolean-literal case to be set up as a keyword followed by appropriate argument(s); that seems like it's enough to prevent syntax conflicts from future additions. So you could imagine \if defined varname \if sql boolean expression to send to server \if compare value operator value It would be easy to allow "not" in front of any one of these, but it's less clear how to do AND or OR combinations. You can always fake AND with nested \if's, but OR is a bit more of a problem. Maybe we don't need it. Other ideas about how to design this? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Corey,
I'm anxious to help with these patches, but they seem a bit of a moving
target. Happy to jump in and review as soon as we've settled on what should
be done.
The "v3" I sent basically adds both client & server version numbers in
client-side variables, basically same code as suggested by Pavel for the
server version, and some documentation.
The questions are:
- which version should be provided (10.0 10 ...)
- how should they be named?
In v3 there is VERSION{,_NAME,_NUM} for client and
SERVER_VERSION_{NUM,NAME} or SVERSION_NUM suggested
by Pavel for server.
- how desirable/useful is it to have this in 10?
Despite that this was not submitted in the relevant CF...
I created an entry in the July one, but this is for 11.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] asynchronous execution
> > > I'll continue working on this from this point aiming to the next > commit fest. > > This probably will not surprise you given the many commits in the past 2 weeks, but the patches no longer apply to master: $ git apply ~/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch /home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:27: trailing whitespace. FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, NULL, 3); /home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:39: trailing whitespace. #include "utils/resowner_private.h" /home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:47: trailing whitespace. ResourceOwner resowner; /* Resource owner */ /home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:48: trailing whitespace. /home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:57: trailing whitespace. WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, NULL, 3); error: patch failed: src/backend/libpq/pqcomm.c:201 error: src/backend/libpq/pqcomm.c: patch does not apply error: patch failed: src/backend/storage/ipc/latch.c:61 error: src/backend/storage/ipc/latch.c: patch does not apply error: patch failed: src/backend/storage/lmgr/condition_variable.c:66 error: src/backend/storage/lmgr/condition_variable.c: patch does not apply error: patch failed: src/backend/utils/resowner/resowner.c:124 error: src/backend/utils/resowner/resowner.c: patch does not apply error: patch failed: src/include/storage/latch.h:101 error: src/include/storage/latch.h: patch does not apply error: patch failed: src/include/utils/resowner_private.h:18 error: src/include/utils/resowner_private.h: patch does not apply
Re: [HACKERS] Variable substitution in psql backtick expansion
On Sun, Apr 2, 2017 at 11:16 AM, Pavel Stehule
wrote:
>
>
> 2017-04-02 13:13 GMT+02:00 Fabien COELHO :
>
>>
>> Hello Pavel,
>>
>> \echo :VERSION
PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
Probably some :VERSION_NUM would make some sense. See attached PoC
patch.
Would it make sense?
>>>
>>> Maybe better name for you CLIENT_VERSION_NUM
>>>
>>
>> If it was starting from nothing I would tend to agree with you, but there
>> is already an existing :VERSION variable, so it seemed logical to keep on
>> and create variants with the same prefix.
>
>
> you have true - so VERSION_NUM should be client side version
>
>
>>
>>
>> Can be SERVER_VERSION_NUM taken from connection info?
>>>
>>
>> Probably it could. It seems a little less straightforward than defining a
>> client-side string at compile time. The information is displayed when the
>> connection is established, so the information is there somewhere.
>>
>
> It is not too hard
>
> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
> index 94a3cfce90..d1ae81646f 100644
> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -3320,16 +3320,21 @@ checkWin32Codepage(void)
> void
> SyncVariables(void)
> {
> + charbuffer[100];
> +
> /* get stuff from connection */
> pset.encoding = PQclientEncoding(pset.db);
> pset.popt.topt.encoding = pset.encoding;
> pset.sversion = PQserverVersion(pset.db);
>
> + snprintf(buffer, 100, "%d", pset.sversion);
> +
> SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
> SetVariable(pset.vars, "USER", PQuser(pset.db));
> SetVariable(pset.vars, "HOST", PQhost(pset.db));
> SetVariable(pset.vars, "PORT", PQport(pset.db));
> SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encod
> ing));
> + SetVariable(pset.vars, "SVERSION_NUM", buffer);
>
> /* send stuff to it, too */
> PQsetErrorVerbosity(pset.db, pset.verbosity);
>
> Regards
>
> Pavel
>
>
>>
>> psql (10devel, server 9.6.2)
>>
>> --
>> Fabien.
>>
>
>
I'm anxious to help with these patches, but they seem a bit of a moving
target. Happy to jump in and review as soon as we've settled on what should
be done.
Re: [HACKERS] Undefined psql variables
On Mon, Jan 23, 2017 at 12:53 PM, Tom Lane wrote: > Corey Huinker writes: > > I was giving some thought to how psql handles undefined variables. > > I would like an option where either psql can provide an alternate value > > when an undefined variable is referenced, or a way to detect that a > > specific variable is undefined and replace it with a defined variable. > > This seems pretty bizarre. What's the use case? Why would it not > be better to build the behavior out of other spare parts, along the > lines of COALESCE or perhaps > > \if not defined(x) > \set x y > \fi > > Obviously the \if stuff is things we don't have yet either, but > it seems less likely to have surprising side-effects. > > regards, tom lane > In light of the backticks variable expansion thread, I'm reviving this thread in the hopes that a defined()-ish psql function can make it into v10. It's something that cannot be solved with a query and \gset, so adding it to psql boolean expressions is the only option I can see.
Re: [HACKERS] Suggested fix for \p and \r in psql
1. \p ignores the "previous buffer". Example: Yeah, I did that intentionally, thinking that the old behavior was confusing. We can certainly discuss it though. I'd tend to agree with your point that \p and \w should print the same thing, but maybe neither of them should look at the previous_buf. After some testing: ISTM that \p should print what \g would execute, otherwise there is no consistent way to look at what \g would do. Currently \e allows to look at this (previous) executed buffer by editing it, but I would find it more consistent if \p is in sync with that, and \e also coldly executes the command on return if it ends with ";". 2. \r keeps the "previous buffer". I think it should clear it. I don't really agree with this. The fact that it used to clear both buffers was an implementation accident that probably nobody had even understood clearly. ISTM that loses functionality because you can't do \g anymore. I agree on this one. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
I wrote: > "Daniel Verite" writes: >> 1. \p ignores the "previous buffer". Example: > Yeah, I did that intentionally, thinking that the old behavior was > confusing. We can certainly discuss it though. I'd tend to agree > with your point that \p and \w should print the same thing, but > maybe neither of them should look at the previous_buf. After a bit more thought, it seems like the definition we want for these is "print what \g would execute, but don't actually change the buffer state". So \w is doing the right thing, \p is not, and that half of your patch is correct. (I'd be inclined to document that spec in a comment in each place, though.) >> 2. \r keeps the "previous buffer". I think it should clear it. > I don't really agree with this. The fact that it used to clear both > buffers was an implementation accident that probably nobody had even > understood clearly. ISTM that loses functionality because you can't > do \g anymore. Still not sure about this. The actual behavior of \r under the old code was to clear query_buf if it was nonempty and otherwise clear previous_buf. It's hard for me to credit that that was intentional, but maybe it was, or at least had behavior natural enough that nobody complained about it. Your patch does strictly more than that, and I think it's too much. What I committed does strictly less; is it too little? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
"Daniel Verite" writes: > I've noticed two issues with the query buffer post-commit e984ef5 > (Support \if ... \elif ... \else ... \endif in psql scripting): > 1. \p ignores the "previous buffer". Example: Yeah, I did that intentionally, thinking that the old behavior was confusing. We can certainly discuss it though. I'd tend to agree with your point that \p and \w should print the same thing, but maybe neither of them should look at the previous_buf. > 2. \r keeps the "previous buffer". I think it should clear it. I don't really agree with this. The fact that it used to clear both buffers was an implementation accident that probably nobody had even understood clearly. ISTM that loses functionality because you can't do \g anymore. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
2017-04-02 13:13 GMT+02:00 Fabien COELHO :
>
> Hello Pavel,
>
> \echo :VERSION
>>> PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
>>> 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
>>>
>>> Probably some :VERSION_NUM would make some sense. See attached PoC patch.
>>> Would it make sense?
>>>
>>
>> Maybe better name for you CLIENT_VERSION_NUM
>>
>
> If it was starting from nothing I would tend to agree with you, but there
> is already an existing :VERSION variable, so it seemed logical to keep on
> and create variants with the same prefix.
you have true - so VERSION_NUM should be client side version
>
>
> Can be SERVER_VERSION_NUM taken from connection info?
>>
>
> Probably it could. It seems a little less straightforward than defining a
> client-side string at compile time. The information is displayed when the
> connection is established, so the information is there somewhere.
>
It is not too hard
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfce90..d1ae81646f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3320,16 +3320,21 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ charbuffer[100];
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
pset.sversion = PQserverVersion(pset.db);
+ snprintf(buffer, 100, "%d", pset.sversion);
+
SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
SetVariable(pset.vars, "USER", PQuser(pset.db));
SetVariable(pset.vars, "HOST", PQhost(pset.db));
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ SetVariable(pset.vars, "SVERSION_NUM", buffer);
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
Regards
Pavel
>
> psql (10devel, server 9.6.2)
>
> --
> Fabien.
>
Re: [HACKERS] Moving GiST index constant to parameter
Thomas Mercieca writes: > I have tried introducing a GUC variable (entry in guc.c/config_real) and I > have also tried having the parser recognize it (same as fillfactor is > recognized), but irrelevant of what I set the value to at run-time, I always > get the same unusual index stats (cannot reproduce results) and the following > messages in the logfile, suggesting that I broke the function: A GUC is certainly not the right thing; you should use an index storage parameter. However, it seems like your problem is independent of that. > "DEBUG: picksplit method for column 1 of index "idx2243" failed > HINT: The index is not optimal. To optimize it, contact a developer, or try > to use the column as the second one in the CREATE INDEX command. > STATEMENT: create index idx2243 on geom_table using gist(geom)" It looks like picksplit would necessarily fail if you tried to use a ratio >= 0.5, or even very close to 0.5, because you'd be demanding unachievable perfection of the split. Beyond that observation, it's hard to say much without seeing what you changed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN cost estimate
- cond := format('%I %s %L', r.colname, r.oper, r.value);
+ cond := format('%s %s %L', r.colname, r.oper, r.value);
Why did you change this? It seems unrelated.
Must be a mistake.
+ indexRel = index_open(index->indexoid, AccessShareLock);
+ pagesPerRange = Min(BrinGetPagesPerRange(indexRel), baserel->pages);
+ Assert(baserel->pages > 0);
+ Assert(pagesPerRange > 0);
+ rangeProportion = (double) pagesPerRange / baserel->pages;
+ numRanges = 1.0 + (double) baserel->pages / pagesPerRange;
+ index_close(indexRel, AccessShareLock);
Why do you add 1.0 to numRanges? This gives one too many ranges.
I have tried to prevent division by zero that can happen later, but your
solution below sounds cleaner to me.
I also don't really like the way you're setting pagesPerRange. I'd suggest
keeping that as the raw value from the index metadata, and doing:
If you want the actual number of ranges then I think this is best expressed
as:
numRanges = Max(ceil(baserel->pages / pagesPerRange), 1.0);
The rangeProportion variable could be calculated based on that
too rangeProportion = 1.0 / numRanges;
That way you don't have to rely on relpages being > 0. It seems like a bad
assumption to make. I see there's some hack in estimate_rel_size() making
that true, but I think it's best not to rely on that hack.
- qual_op_cost = cpu_operator_cost *
- (list_length(indexQuals) + list_length(indexOrderBys));
-
*indexStartupCost += qual_arg_cost;
*indexTotalCost += qual_arg_cost;
- *indexTotalCost += (numTuples * *indexSelectivity) *
(cpu_index_tuple_cost + qual_op_cost);
*indexPages = index->pages;
- /* XXX what about pages_per_range? */
+ /*
+ * Add index qual op costs. Unlike other indexes, we are not processing
+ * tuples but ranges.
+ */
+ qual_op_cost = cpu_operator_cost * list_length(indexQuals);
+ *indexTotalCost += numRanges * qual_op_cost;
What's the reason for removing the + list_length(indexOrderBys) here? I
don't think it's the business of this patch to touch that. BRIN may one day
support that by partition sorting non-overlapping ranges, so that could
well be why it was there in the first place.
I thought it was a mistake. I agree we better not change it, even though I
have no idea how cost of BRIN sorting can be calculated. I guess the
indexOrderBys list would always be empty for now, anyway.
- *indexTotalCost += (numTuples * *indexSelectivity) *
(cpu_index_tuple_cost + qual_op_cost);
+ *indexTotalCost += selec * numTuples * cpu_index_tuple_cost;
Why is this not being charged qual_op_cost anymore?
I must have thought that BRIN doesn't execute the qual unlike btree. I am
not sure what is the best equation to put there. Previously, we were
returning good selectivity, but high cost. I believe things should be
other way around. Maybe what we really need is to use "numRanges" in here
instead of "selec * numTuples".
+ * Our starting point is that BRIN selectivity has to be less than the
+ * selectivity of the btree. We are using a product of logical and
Can you explain why this is the case?
My wording sounds wrong in here. I should have said "worse than" instead
of "less than".
+ * class "minmax", and makes a little sense for the other one "inclusion".
"and" I think should be "but"
I agree.
I think it would also be good to write some regression tests for this. All
the changes I see that you did make to brin.sql seem unrelated to this
patch.
I added an expression index to test getting correlation of expressions.
The BRIN tests would call the estimation functions, but we don't have any
tests to check the result of the functions. We can maybe prepare a test to
check BRIN is prefferred over btree when it would perform better, and maybe
also vice versa.
Unfortunately, I am on vacation for two weeks without my computer. I can
post another version after 18th. I know we are under time pressure for
release. I wouldn't mind if you or Alvaro would change it anyway you like.
Re: [HACKERS] Performance improvement for joins where outer side is unique
David Rowley writes: > Tom, I'm wondering if you think you'll get time to look at this before the > feature freeze? Yeah, I intend to. Thanks for updating the patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Suggested fix for \p and \r in psql
Hello Daniel, I've noticed two issues with the query buffer post-commit e984ef5 (Support \if ... \elif ... \else ... \endif in psql scripting): I thought that Tom's changes were somehow intentional, in order to simplify the code. 1. \p ignores the "previous buffer". Example: postgres=# select 1; ?column? -- 1 (1 row) postgres=# \p Query buffer is empty. Yep. Note that it still works with: SELECT 1 \g That doesn't match the pre-commit behavior, and is not consistent with \e or \w Indeed, there is definitely an issue because \g executes something where \p prints nothing... 10devel> SELECT 1; -- 1 10devel> \p -- 10devel> \g -- 1! 2. \r keeps the "previous buffer". I think it should clear it. Example: I'm not that sure, \r clears the current buffer and restores the previous one, so that two \r are needed to fully clear: 9.6> SELECT 1; -- 1 9.6> SELECT 2 \r -- first clear 9.6> \g -- 1 (redo SELECT 1) 9.6> \r -- second clear 9.6> \g -- Patch applies, make check ok. However: I tend to agree that 1 above is a regression, but ISTM that 2 is somehow the expected behavior, i.e. \r should not clear the previous buffer, just the current one, so that two \r are needed for a "full" clear. \r should clear the current buffer and restores the previous one. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [GSoC] Question about "Explicitly support predicate locks in index access methods besides btree"
Hi guys, I'm confused about the this function, CheckForSerializableConflictOut(...). Can anyone help me out? It seems like this function is used to check the rw-antidependencies out edge. This should be done when reading a written tuple. But btree does not call this function at all. The heapam used predicatelockpage(...)... I use to think the in and out edge will be built when rw-antidependencies detected. CheckForSerializableConflictIn(...) can do this job. So CheckForSerializableConflictOut(...) function really confused me. Best Wishes! --- Dong
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Daniel, SELECT some-boolean-expression AS okay \gset \if :okay Yes, the question was whether we leave it as that for v10, or if it's worth a last-minute improvement for usability, assuming it's doable, similarly to what $subject does to backtick expansion for external evaluation. My 0.02 € about server-side expressions: ISTM that there is nothing obvious/easy to do to include these: - how would it work, both with \set ... and \if ...? - should it be just simple expressions or may it allow complex queries? - how would error detection and handling work from a script? - should it have some kind of continuation, as expressions are likely to be longer than a constant? - how would they interact with possible client-side expressions? (on this point, I think that client-side is NOT needed for "psql". It makes sense for "pgbench" in a benchmarking context where the client must interact with the server in some special meaningful way, but for simple scripting the performance requirement and logic is not the same, so server-side could be enough). Basically quite a few questions which would not find an instantaneous answer and associated patch. However I agree with you that there may be minimal usability things to add before 10, similar to Tom's backtick variable substitution. Having some access to the client version as suggested by Pavel looks like a good idea for the kind of script which may rely on conditionals... Maybe other things, not sure what, though. Maybe other client settings could be exported as variables, but the version looks like the main which is currently missing. Maybe a way to know what is the client current status? eg in transaction, transaction has aborted, things like that? -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Fabien COELHO wrote: > Note that this is already available indirectly, as show in the > documentation. > > SELECT some-boolean-expression AS okay \gset > \if :okay > \echo boolean expression was true > \else > \echo boolean expression was false > \endif Yes, the question was whether we leave it as that for v10, or if it's worth a last-minute improvement for usability, assuming it's doable, similarly to what $subject does to backtick expansion for external evaluation. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: http://www.manitou-mail.org Twitter: @DanielVerite -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] FDW and parallel execution
Hi hackers and personally Robet (you are the best expert in both areas).
I want to ask one more question concerning parallel execution and FDW.
Below are two plans for the same query (TPC-H Q5): one for normal tables,
another for FDW to vertical representation of the same data.
FDW supports analyze function and is expected to produce the similar statistic
as for original tables.
Query plans are the following:
Normal tables:
Sort (cost=2041588.48..2041588.98 rows=200 width=48)
Sort Key: (sum((lineitem.l_extendedprice * ('1'::double precision -
lineitem.l_discount DESC
-> Finalize GroupAggregate (cost=2041335.76..2041580.83 rows=200 width=48)
Group Key: nation.n_name
-> Gather Merge (cost=2041335.76..2041568.33 rows=1400 width=48)
Workers Planned: 7
-> Partial GroupAggregate (cost=2040335.64..2040396.71 rows=200
width=48)
Group Key: nation.n_name
-> Sort (cost=2040335.64..2040345.49 rows=3938 width=40)
Sort Key: nation.n_name
-> Hash Join (cost=605052.97..2040100.48 rows=3938
width=40)
Hash Cond: ((orders.o_custkey =
customer.c_custkey) AND (nation.n_nationkey = customer.c_nationkey))
-> Hash Join (cost=525126.37..1951647.85
rows=98414 width=52)
Hash Cond: (lineitem.l_orderkey =
orders.o_orderkey)
-> Hash Join (cost=3802.22..1404473.37
rows=654240 width=52)
Hash Cond: (lineitem.l_suppkey =
supplier.s_suppkey)
-> Parallel Seq Scan on lineitem
(cost=0.00..1361993.36 rows=8569436 width=16)
-> Hash (cost=3705.97..3705.97
rows=7700 width=44)
-> Hash Join
(cost=40.97..3705.97 rows=7700 width=44)
Hash Cond:
(supplier.s_nationkey = nation.n_nationkey)
-> Seq Scan on
supplier (cost=0.00..3090.00 rows=10 width=8)
-> Hash
(cost=40.79..40.79 rows=15 width=36)
-> Hash Join (cost=20.05..40.79 rows=15 width=36)
Hash Cond: (nation.n_regionkey = region.r_regionkey)
-> Seq Scan on nation (cost=0.00..17.70 rows=770 width=40)
-> Hash (cost=20.00..20.00 rows=4 width=4)
-> Seq Scan on region (cost=0.00..20.00 rows=4 width=4)
Filter: ((r_name)::text = 'ASIA'::text)
-> Hash (cost=484302.37..484302.37
rows=2256542 width=8)
-> Seq Scan on orders
(cost=0.00..484302.37 rows=2256542 width=8)
Filter: ((o_orderdate >=
'1996-01-01'::date) AND (o_orderdate < '1997-01-01'::date))
-> Hash (cost=51569.64..51569.64 rows=1499864
width=8)
-> Seq Scan on customer
(cost=0.00..51569.64 rows=1499864 width=8)
Plan with FDW:
Sort (cost=2337312.28..2337312.78 rows=200 width=48)
Sort Key: (sum((lineitem_fdw.l_extendedprice * ('1'::double precision -
lineitem_fdw.l_discount DESC
-> GroupAggregate (cost=2336881.54..2337304.64 rows=200 width=48)
Group Key: nation.n_name
-> Sort (cost=2336881.54..2336951.73 rows=28073 width=40)
Sort Key: nation.n_name
-> Hash Join (cost=396050.65..2334807.39 rows=28073 width=40)
Hash Cond: ((orders_fdw.o_custkey =
customer_fdw.c_custkey) AND (nation.n_nationkey = customer_fdw.c_nationkey))
-> Hash Join (cost=335084.53..2247223.46 rows=701672
width=52)
Hash Cond: (lineitem_fdw.l_orderkey =
orders_fdw.o_orderkey)
-> Hash Join (cost=2887.07..1786058.18 rows=4607421
width=52)
Hash Cond: (lineitem_fdw.l_suppkey =
supplier_fdw.s_suppkey)
-> Foreign Scan on lineitem_fdw
(cost=0.00..1512151.52 rows=59986176 width=16)
-> Hash (cost=2790.80..2790.80 rows=7702
width=44)
-> Hash Join (cost=40.97..2790.80
rows=7702 width=44)
Hash Cond:
(supplier_fdw.s_nationkey = nation.n_nationkey)
-> Foreign Scan on supplier_fdw
(cost=0.00..2174.64 rows=100032 width=8)
-> Hash (cost=40.79..40.79
rows=15 width=36)
-> Hash Join
(cost=20.05..40.79 rows=15 width=36)
Hash Cond:
(nation.n_reg
[HACKERS] Suggested fix for \p and \r in psql
Hi,
I've noticed two issues with the query buffer post-commit e984ef5
(Support \if ... \elif ... \else ... \endif in psql scripting):
1. \p ignores the "previous buffer". Example:
postgres=# select 1;
?column?
--
1
(1 row)
postgres=# \p
Query buffer is empty.
That doesn't match the pre-commit behavior, and is not
consistent with \e or \w
2. \r keeps the "previous buffer". I think it should clear it. Example:
postgres=# select 1;
?column?
--
1
(1 row)
postgres=# select 2 \r
Query buffer reset (cleared).
postgres=# \w /tmp/buffer
postgres=# \! cat /tmp/buffer
select 1;
I suggest the attached fix, with a few new regression tests.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..6554268 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -106,14 +106,14 @@ static backslashResult exec_command_lo(PsqlScanState scan_state, bool active_bra
const char *cmd);
static backslashResult exec_command_out(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_print(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf);
+ PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_password(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_prompt(PsqlScanState scan_state, bool active_branch,
const char *cmd);
static backslashResult exec_command_pset(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_quit(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf);
+ PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_s(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_set(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_setenv(PsqlScanState scan_state, bool active_branch,
@@ -192,8 +192,8 @@ static void checkWin32Codepage(void);
* execution of the backslash command (for example, \r clears it).
*
* previous_buf contains the query most recently sent to the server
- * (empty if none yet). This should not be modified here, but some
- * commands copy its content into query_buf.
+ * (empty if none yet). This should not be modified here (except by
+ * \r), but some commands copy its content into query_buf.
*
* query_buf and previous_buf will be NULL when executing a "-c"
* command-line option.
@@ -362,7 +362,8 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
status = exec_command_out(scan_state, active_branch);
else if (strcmp(cmd, "p") == 0 || strcmp(cmd, "print") == 0)
- status = exec_command_print(scan_state, active_branch, query_buf);
+ status = exec_command_print(scan_state, active_branch,
+ query_buf, previous_buf);
else if (strcmp(cmd, "password") == 0)
status = exec_command_password(scan_state, active_branch);
else if (strcmp(cmd, "prompt") == 0)
@@ -372,7 +373,8 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "q") == 0 || strcmp(cmd, "quit") == 0)
status = exec_command_quit(scan_state, active_branch);
else if (strcmp(cmd, "r") == 0 || strcmp(cmd, "reset") == 0)
- status = exec_command_reset(scan_state, active_branch, query_buf);
+ status = exec_command_reset(scan_state, active_branch,
+ query_buf, previous_buf);
else if (strcmp(cmd, "s") == 0)
status = exec_command_s(scan_state, active_branch);
else if (strcmp(cmd, "set") == 0)
@@ -1827,12 +1829,15 @@ exec_command_out(PsqlScanState scan_state, bool active_branch)
*/
static backslashResult
exec_command_print(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf)
+ PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (active_branch)
{
if (query_buf && query_buf->len > 0)
puts(query_buf->data);
+ /* Applies to previous query if current buffer is empty */
+ else if (previous_buf && previous_buf->len > 0)
+ puts(previous_buf->data);
else if (!pset.quiet)
puts(_("Query buffer is empty."));
fflush(stdout);
@@ -2056,10 +2061,11 @@ exec_command_quit(PsqlScanState scan_state, bool active_branch)
*/
static backslashResult
exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf)
+ PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (active_branch)
{
+ resetPQExpBuffer(previous_buf);
resetPQExpBuffer(query_buf);
psql_scan_reset(scan_state);
if (!pset.quiet)
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8aa914f..f1c516a 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2932,3 +2932,30 @@ NOTICE:
Re: [HACKERS] Variable substitution in psql backtick expansion
Can be SERVER_VERSION_NUM taken from connection info?
Here is a version with that, quite easy as well as the information was
already there for other checks.
I have not updated "help.c" because the initial VERSION was not presented
there in the first place.
Here is a v3 to fix the documentation example. Sorry for the noise.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ad463e7..b1508be 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3580,6 +3580,18 @@ bar
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+These variables are set when connected to reflects the server's version
+both in short name (eg "9.6.2", "10.0") and number (eg 90602, 10).
+They can be changed or unset.
+
+
+
+
+
SINGLELINE
@@ -3625,10 +3637,13 @@ bar
VERSION
+VERSION_NAME
+VERSION_NUM
-This variable is set at program start-up to
-reflect psql's version. It can be changed or unset.
+These variables are set at program start-up to reflect
+psql's version in verbose, short name ("10.0")
+and number (10). They can be changed or unset.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..2c58cfb 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3320,6 +3320,8 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ char vbuf[32];
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
@@ -3331,6 +,12 @@ SyncVariables(void)
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ /* server version information */
+ SetVariable(pset.vars, "SERVER_VERSION_NAME",
+formatPGVersionNumber(pset.sversion, true, vbuf, sizeof(vbuf)));
+ snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3349,6 +3357,8 @@ UnsyncVariables(void)
SetVariable(pset.vars, "HOST", NULL);
SetVariable(pset.vars, "PORT", NULL);
SetVariable(pset.vars, "ENCODING", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 8068a28..91d6a8f 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,11 @@ main(int argc, char *argv[])
EstablishVariableSpace();
SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+ SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+
+#define STRINGIFY2(symbol) #symbol
+#define STRINGIFY(symbol) STRINGIFY2(symbol)
+ SetVariable(pset.vars, "VERSION_NUM", STRINGIFY(PG_VERSION_NUM));
/* Default values for variables (that don't match the result of \unset) */
SetVariableBool(pset.vars, "AUTOCOMMIT");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Can be SERVER_VERSION_NUM taken from connection info?
Here is a version with that, quite easy as well as the information was
already there for other checks.
I have not updated "help.c" because the initial VERSION was not presented
there in the first place.
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ad463e7..5046385 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3580,6 +3580,18 @@ bar
+SERVER_VERSION_NAME
+SERVER_VERSION_NUM
+
+
+These variables are set when connected to reflects the server's version
+both in short name ("10") and number (10).
+They can be changed or unset.
+
+
+
+
+
SINGLELINE
@@ -3625,10 +3637,13 @@ bar
VERSION
+VERSION_NAME
+VERSION_NUM
-This variable is set at program start-up to
-reflect psql's version. It can be changed or unset.
+These variables are set at program start-up to reflect
+psql's version in verbose, short name ("10") and number (10).
+They can be changed or unset.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..2c58cfb 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3320,6 +3320,8 @@ checkWin32Codepage(void)
void
SyncVariables(void)
{
+ char vbuf[32];
+
/* get stuff from connection */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
@@ -3331,6 +,12 @@ SyncVariables(void)
SetVariable(pset.vars, "PORT", PQport(pset.db));
SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encoding));
+ /* server version information */
+ SetVariable(pset.vars, "SERVER_VERSION_NAME",
+formatPGVersionNumber(pset.sversion, true, vbuf, sizeof(vbuf)));
+ snprintf(vbuf, sizeof(vbuf), "%d", pset.sversion);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", vbuf);
+
/* send stuff to it, too */
PQsetErrorVerbosity(pset.db, pset.verbosity);
PQsetErrorContextVisibility(pset.db, pset.show_context);
@@ -3349,6 +3357,8 @@ UnsyncVariables(void)
SetVariable(pset.vars, "HOST", NULL);
SetVariable(pset.vars, "PORT", NULL);
SetVariable(pset.vars, "ENCODING", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NAME", NULL);
+ SetVariable(pset.vars, "SERVER_VERSION_NUM", NULL);
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 8068a28..91d6a8f 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,11 @@ main(int argc, char *argv[])
EstablishVariableSpace();
SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+ SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+
+#define STRINGIFY2(symbol) #symbol
+#define STRINGIFY(symbol) STRINGIFY2(symbol)
+ SetVariable(pset.vars, "VERSION_NUM", STRINGIFY(PG_VERSION_NUM));
/* Default values for variables (that don't match the result of \unset) */
SetVariableBool(pset.vars, "AUTOCOMMIT");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN cost estimate
On 27 March 2017 at 00:44, Emre Hasegeli wrote:
> > If we want to have a variable which stores the number of ranges, then
> > I think numRanges is better than numBlocks. I can't imagine many
> > people would disagree there.
>
> I renamed it with other two.
>
> > At the very least please write a comment to explain this in the code.
> > Right now it looks broken. If I noticed this then one day in the
> > future someone else will. If you write a comment then person of the
> > future will likely read it, and then not raise any questions about the
> > otherwise questionable code.
>
> I added a sentence about it.
>
> > I do strongly agree that the estimates need improved here. I've
> > personally had issues with bad brin estimates before, and I'd like to
> > see it improved. I think the patch is not quite complete without it
> > also considering stats on expression indexes. If you have time to go
> > do that I'd suggest you go ahead with that.
>
> I copy-pasted expression index support from btcostestimate() as well,
> and extended the regression test.
>
> I think this function can use more polishing before committing, but I
> don't know where to begin. There are single functions for every
> access method in here. I don't like them being in there to begin
> with. selfuncs.s is quite long with all kinds of dependencies and
> dependents. I think it would be better to have the access method
> selectivity estimation functions under src/access. Maybe we should
> start doing so by moving this to src/access/brin/brin_selfuncs.c.
> What do you think?
>
Looking over this again.
- cond := format('%I %s %L', r.colname, r.oper, r.value);
+ cond := format('%s %s %L', r.colname, r.oper, r.value);
Why did you change this? It seems unrelated.
+ indexRel = index_open(index->indexoid, AccessShareLock);
+ pagesPerRange = Min(BrinGetPagesPerRange(indexRel), baserel->pages);
+ Assert(baserel->pages > 0);
+ Assert(pagesPerRange > 0);
+ rangeProportion = (double) pagesPerRange / baserel->pages;
+ numRanges = 1.0 + (double) baserel->pages / pagesPerRange;
+ index_close(indexRel, AccessShareLock);
Why do you add 1.0 to numRanges? This gives one too many ranges.
I also don't really like the way you're setting pagesPerRange. I'd suggest
keeping that as the raw value from the index metadata, and doing:
If you want the actual number of ranges then I think this is best expressed
as:
numRanges = Max(ceil(baserel->pages / pagesPerRange), 1.0);
The rangeProportion variable could be calculated based on that
too rangeProportion = 1.0 / numRanges;
That way you don't have to rely on relpages being > 0. It seems like a bad
assumption to make. I see there's some hack in estimate_rel_size() making
that true, but I think it's best not to rely on that hack.
- qual_op_cost = cpu_operator_cost *
- (list_length(indexQuals) + list_length(indexOrderBys));
-
*indexStartupCost += qual_arg_cost;
*indexTotalCost += qual_arg_cost;
- *indexTotalCost += (numTuples * *indexSelectivity) *
(cpu_index_tuple_cost + qual_op_cost);
*indexPages = index->pages;
- /* XXX what about pages_per_range? */
+ /*
+ * Add index qual op costs. Unlike other indexes, we are not processing
+ * tuples but ranges.
+ */
+ qual_op_cost = cpu_operator_cost * list_length(indexQuals);
+ *indexTotalCost += numRanges * qual_op_cost;
What's the reason for removing the + list_length(indexOrderBys) here? I
don't think it's the business of this patch to touch that. BRIN may one day
support that by partition sorting non-overlapping ranges, so that could
well be why it was there in the first place.
- *indexTotalCost += (numTuples * *indexSelectivity) *
(cpu_index_tuple_cost + qual_op_cost);
+ *indexTotalCost += selec * numTuples * cpu_index_tuple_cost;
Why is this not being charged qual_op_cost anymore?
+ * Our starting point is that BRIN selectivity has to be less than the
+ * selectivity of the btree. We are using a product of logical and
Can you explain why this is the case?
+ * class "minmax", and makes a little sense for the other one "inclusion".
"and" I think should be "but"
I think it would also be good to write some regression tests for this. All
the changes I see that you did make to brin.sql seem unrelated to this
patch.
I've also attached a spreadsheet that can be used to play around with the
estimates your patch is giving.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
BRIN_estimates2.ods
Description: application/vnd.oasis.opendocument.spreadsheet
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance improvement for joins where outer side is unique
On 2 April 2017 at 21:21, David Rowley wrote: > I've attached an updated patch which updates the regression test output of > a recent commit to include the "Unique Inner" in the expected results. > The patch must've fallen off. Attempt number 2 at attaching. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services unique_joins_2017-04-02.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Performance improvement for joins where outer side is unique
On Sun, Apr 2, 2017 at 5:21 AM, David Rowley wrote: > I've attached an updated patch which updates the regression test output of a > recent commit to include the "Unique Inner" in the expected results. Was this email supposed to have a patch attached? > Tom, I'm wondering if you think you'll get time to look at this before the > feature freeze? /me crosses fingers. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Pavel, \echo :VERSION PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit Probably some :VERSION_NUM would make some sense. See attached PoC patch. Would it make sense? Maybe better name for you CLIENT_VERSION_NUM If it was starting from nothing I would tend to agree with you, but there is already an existing :VERSION variable, so it seemed logical to keep on and create variants with the same prefix. Can be SERVER_VERSION_NUM taken from connection info? Probably it could. It seems a little less straightforward than defining a client-side string at compile time. The information is displayed when the connection is established, so the information is there somewhere. psql (10devel, server 9.6.2) -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
2017-04-02 9:45 GMT+02:00 Fabien COELHO : > > Hello Pavel, > > For this case can be nice to have function that returns server version as >> number some like version_num() .. 1 >> > > The server side information can be queried: > > SELECT current_setting(‘server_version_num’) > AS server_version_num \gset > -- 90602 > > However client side is not so clean: > > \echo :VERSION > PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu > 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit > > Probably some :VERSION_NUM would make some sense. See attached PoC patch. > Would it make sense? Maybe better name for you CLIENT_VERSION_NUM Can be SERVER_VERSION_NUM taken from connection info? Regards Pavel > > > -- > Fabien.
Re: [HACKERS] Variable substitution in psql backtick expansion
2017-04-02 9:45 GMT+02:00 Fabien COELHO : > > Hello Pavel, > > For this case can be nice to have function that returns server version as >> number some like version_num() .. 1 >> > > The server side information can be queried: > > SELECT current_setting(‘server_version_num’) > AS server_version_num \gset > -- 90602 > > However client side is not so clean: > > \echo :VERSION > PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu > 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit > > Probably some :VERSION_NUM would make some sense. See attached PoC patch. > Would it make sense? It has sense Pavel > > > -- > Fabien.
[HACKERS] Moving GiST index constant to parameter
Hi all, The GiST index has a picksplit support procedure (in gistproc.c) with a constant set up using #define - LIMIT_RATIO, set to 0.3. The PostGIS extension which shares similar code for GiST has this set to 0.1 (gserialized_gist_picksplit_2d.c). Compiling new builds, I could see a difference in my indexes with different values. I would like to introduce it as a parameter so the procedure will behave according to the user's defined value at run-time. In other words, different GiST indexes would have different values of LIMIT_RATIO without requiring a new PostgreSQL/PostGIS build. However, I am not sure what the best approach is. I have tried introducing a GUC variable (entry in guc.c/config_real) and I have also tried having the parser recognize it (same as fillfactor is recognized), but irrelevant of what I set the value to at run-time, I always get the same unusual index stats (cannot reproduce results) and the following messages in the logfile, suggesting that I broke the function: "DEBUG: picksplit method for column 1 of index "idx2243" failed HINT: The index is not optimal. To optimize it, contact a developer, or try to use the column as the second one in the CREATE INDEX command. STATEMENT: create index idx2243 on geom_table using gist(geom)" and also "DEBUG: mapped win32 error code 2 to 2 STATEMENT: create index idx2243 on geom_table using gist(geom)" I was thinking that picksplit would always look at an updated version of LIMIT_RATIO. But I'm at a loss at how to approach the problem at this stage. Perhaps my implementation is not correct or my approach not appropriate at all. Is there a better way of introducing this parameter? Or maybe I am missing something? I would be very grateful for any feedback, pointers or comments. Best regards, Thomas
Re: [HACKERS] Performance improvement for joins where outer side is unique
On 27 March 2017 at 15:51, David Rowley wrote: > On 27 March 2017 at 09:28, David Rowley > wrote: > > > Patch is attached which fixes up the conflict between the expression > > evaluation performance patch. > > Seems I forgot to commit locally before creating the patch... Here's > the actual patch I meant to attach earlier. I've attached an updated patch which updates the regression test output of a recent commit to include the "Unique Inner" in the expected results. I've also performed a pgindent run on the patch. Tom, I'm wondering if you think you'll get time to look at this before the feature freeze? David -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Variable substitution in psql backtick expansion
For this case can be nice to have function that returns server version as number some like version_num() .. 1 Another possible trick to get out of a script which does not support \if, relying on the fact that the unexpected command is simply ignored: -- exit version below 10 \if false \echo 'script requires version 10 or better' \q \endif Also possible but less informative on errors: \set ON_ERROR_STOP on \if false \endif -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
Hello Pavel,
For this case can be nice to have function that returns server version
as number some like version_num() .. 1
The server side information can be queried:
SELECT current_setting(‘server_version_num’)
AS server_version_num \gset
-- 90602
However client side is not so clean:
\echo :VERSION
PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
Probably some :VERSION_NUM would make some sense. See attached PoC patch.
Would it make sense?
--
Fabien.diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ad463e7..adc6a47 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3625,10 +3625,13 @@ bar
VERSION
+VERSION_NAME
+VERSION_NUM
-This variable is set at program start-up to
-reflect psql's version. It can be changed or unset.
+These variable are set at program start-up to reflect
+psql's version in verbose, short name ("10") and number (10).
+They can be changed or unset.
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 8068a28..91d6a8f 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -161,6 +161,11 @@ main(int argc, char *argv[])
EstablishVariableSpace();
SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
+ SetVariable(pset.vars, "VERSION_NAME", PG_VERSION);
+
+#define STRINGIFY2(symbol) #symbol
+#define STRINGIFY(symbol) STRINGIFY2(symbol)
+ SetVariable(pset.vars, "VERSION_NUM", STRINGIFY(PG_VERSION_NUM));
/* Default values for variables (that don't match the result of \unset) */
SetVariableBool(pset.vars, "AUTOCOMMIT");
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Variable substitution in psql backtick expansion
2017-04-02 8:53 GMT+02:00 Fabien COELHO : > > Bonjour Daniel, > > I think that users would rather have the option to just put >> an SQL expression behind \if. >> > > Note that this is already available indirectly, as show in the > documentation. > > SELECT some-boolean-expression AS okay \gset > \if :okay > \echo boolean expression was true > \else > \echo boolean expression was false > \endif > > For this case can be nice to have function that returns server version as number some like version_num() .. 1 comments? Regards Pavel > -- > Fabien. > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
