Re: csv format for psql
2018-01-31 14:09 GMT+01:00 Pavel Stehule :
>
>
> 2018-01-31 13:58 GMT+01:00 Daniel Verite :
>
>> Pavel Stehule wrote:
>>
>> > This format is too important, so some special short or long option can
>> be
>> > practical (it will be printed in help)
>> >
>> > some like --csv
>>
>> I guess -C/--csv could be used, like there already is -H/--html.
>>
>> > I found one issue - PostgreSQL default field separator is "|". Maybe
>> good
>> > time to use more common "," ?
>> >
>> > Or when field separator was not explicitly defined, then use "," for
>> CSV,
>> > and "|" for other. Although it can be little bit messy
>>
>> Currently there's a strong analogy between the unaligned
>> mode and csv mode. In particular they use the existing pset
>> variables fieldsep, fieldsep_zero, recordsep, recordsep_zero
>> in the same way. If we want to make csv special with regard
>> to the delimiters, that complicates the user interface
>> For instance if fieldsep was changed automatically by
>> \pset format csv, it's not obvious if/when we should switch it
>> back to its previous state, and how the fieldsep switch done
>> automatically would mix or conflict with other \pset
>> commands issued by the user.
>> Or we need to duplicate these variables. Or duplicate
>> only fieldsep, having a fieldsep_csv, leaving out the
>> other variables and not being as close to the unaligned
>> mode.
>> These options don't appeal to me much compared
>> to the simplicity of the current patch.
>>
>> Also, although the comma is the separator defined by the
>> RFC4180 and the default for COPY CSV, people also use the
>> semicolon extensively (because it's what Excel does I guess),
>> which somehow mitigates the importance of comma as the
>> default value.
>>
>>
> The functionality is clean and great - default setting "|" is less. I have
> not strong opinion about it and I understand to your arguments. Has anybody
> some idea?
>
two other ideas from me
1. currently \pset format xxx command has not any other parameters. Other
optional parameters can be set of possible attributes like {, ; | \t
header_off escape_all}. If this set will be known by tabcomplete, then it
can be very friendly and I don't think so code will be more complex than in
your proposal (long options can be "--csv --csv-options=", header_off" )
some like "\pset format csv , header_off
or just limited variant with only field separator specification
\pset format csv ;
2. if we support csv format, then we can introduce \gcsv xxx | gnuplot
It is just idea.
@1 is much more interesting than @2
Regards
Pavel
>
> Regards
>
> Pavel
>
>
>>
>> Best regards,
>> --
>> Daniel Vérité
>> PostgreSQL-powered mailer: http://www.manitou-mail.org
>> Twitter: @DanielVerite
>>
>
>
Typo with pg_multixact/offset in multixact.c
Hi all, While hacking some stuff, I bumped into the following: --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1932,7 +1932,7 @@ ZeroMultiXactMemberPage(int pageno, bool writeXlog) * MaybeExtendOffsetSlru * Extend the offsets SLRU area, if necessary * - * After a binary upgrade from <= 9.2, the pg_multixact/offset SLRU area might + * After a binary upgrade from <= 9.2, the pg_multixact/offsets SLRU area might * contain files that are shorter than necessary; this would occur if the old * installation had used multixacts beyond the first page (files cannot be * copied, because the on-disk representation is different). pg_upgrade would Thanks, -- Michael diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 6d6f2e3016..a9a51055e9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1932,7 +1932,7 @@ ZeroMultiXactMemberPage(int pageno, bool writeXlog) * MaybeExtendOffsetSlru * Extend the offsets SLRU area, if necessary * - * After a binary upgrade from <= 9.2, the pg_multixact/offset SLRU area might + * After a binary upgrade from <= 9.2, the pg_multixact/offsets SLRU area might * contain files that are shorter than necessary; this would occur if the old * installation had used multixacts beyond the first page (files cannot be * copied, because the on-disk representation is different). pg_upgrade would signature.asc Description: PGP signature
Using base backup exclusion filters to reduce data transferred with pg_rewind
Hi all, Many threads have touched $subject: https://www.postgresql.org/message-id/CAN-RpxDPE4baiMMJ6TLd6AiUvrG=yrc05tgxrgp4auuth9j...@mail.gmail.com https://www.postgresql.org/message-id/7c50423.5ad0.15e8b308b2f.coremail.chjis...@163.com https://www.postgresql.org/message-id/1516736993.5599.4.ca...@cybertec.at Thread [2] is a bit different as it discusses with WAL segments data which is useless at the end, still it aimed at reducing the amount of data transferred during a rewind. I am not tackling this problem in the patch set of this thread. This can be discussed separately. Attached is a patch set which implements what I have mentioned a couple of times in those threads by having pg_rewind reuse the same exclusion filtering rules as for base backups, as after a rewind a node enters in recovery and a bunch of data which is copied during the rewind finishes in the void. There has been as well many complains about the need to remove all the time replication slot data manually after a rewind, so I believe that this makes the user experience much easier with the tool. Something useful is replication slot data getting filtered out. In order to reach this point, I have been hacking the backend code and finished with quite a bit of shuffling around how system paths are hardcoded in many places, like pg_replslot, pg_wal, etc. Well you can think here about all the paths hardcoded in initdb.c. So I have introduced a couple of things: - src/include/pg_paths.h, a new header which gathers the set of system file and directory names. With this facility in place, the backend code loses knowledge of hardcoded system-related paths, including things like "base", "global", "pg_tblspc", "base/pg_control", etc. A fun consequence of that refactoring is that it is possible to just change pg_paths.h and have change the system paths of a PostgreSQL instance with one-liners. For example you could change "base/" to "foo/". This can make PostgreSQL more malleable for forks. It would be more simple to just hardcode more the paths but I think that this would not be manageable in the long-term, especially if similar logics spread more. - src/include/replication/basebackup_paths.h, which extracts the exclude rules now in basebackup.c into a header which can be consumed by both frontends and backends. This is useful for any backup tools. - pg_rewind update to ensure that the filters are working correctly. So the patch set attached is made of the following: - 0001, which refactors all hardcoded system paths into pg_paths.h. This modifies only initdb.c and basebackup.c to ease reviews. - 0002 spreads the path changes and the use of pg_paths.h across the core code. - 0003 moves the last set of definitions with backup_label, tablespace_map and pg_internal.init. - 0004 creates basebackup_paths.h, this can be consumed by pg_rewind. - 0005 makes the changes for pg_rewind. 0001~0003 can be merged together, I have just done a split to ease reviews. I am adding that to the next CF. Thanks, -- Michael From 6698d430ffb6bd2e33aba0b21dc2a9358cf6328c Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 5 Feb 2018 13:03:38 +0900 Subject: [PATCH 1/5] Refactor path definitions into a single header file, pg_paths.h The definition of all internal paths of PostgreSQL are spread across the code base, making tracing of those definitions difficult to begin with, and resulting in a lot of code paths to be hardcoded. While this has no real practical consequences in practice, this makes the creation of lists manipulating those paths less maintainable as as things stand the already-hardcoded paths need to be copied around more. In order to improve things for the long-term, move all those definitions into a single header file. This commit does a first step by switching basebackup.c and initdb.c to use them. An upcoming commit will make all the definitions across the code base use this new header more consistently. --- src/backend/postmaster/postmaster.c | 1 + src/backend/postmaster/syslogger.c | 1 + src/backend/replication/basebackup.c | 16 ++-- src/backend/storage/ipc/dsm.c| 1 + src/backend/storage/ipc/dsm_impl.c | 1 + src/backend/storage/lmgr/predicate.c | 3 +- src/backend/utils/adt/misc.c | 1 + src/bin/initdb/initdb.c | 45 ++-- src/include/access/xlog_internal.h | 7 +- src/include/pg_paths.h | 137 +++ src/include/pgstat.h | 3 - src/include/postmaster/syslogger.h | 7 -- src/include/storage/dsm_impl.h | 9 --- src/include/utils/guc.h | 7 -- 14 files changed, 176 insertions(+), 63 deletions(-) create mode 100644 src/include/pg_paths.h diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index f3ddf828bb..66d80914a0 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -105,6 +105,7 @@ #include "libp
Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis
Hi, Thank you all for this so warm reception for my very first patch. Thanks Stephen to have thought to add that patch to the commit fest. As it was committed Friday, I was able to tell the whole story in my talk and that's great. And thanks again to everyone who helped me with that patch. Regards Lætitia Le 5 févr. 2018 01:36, "Amit Langote" a écrit : > On 2018/02/02 19:41, Stephen Frost wrote: > > * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote: > >> On 2018/01/23 8:57, Thomas Munro wrote: > >>> Here's an update to move the new stuff to the correct side of the > >>> closing "". Builds for me, and the typesetting looks OK. > >>> I'm not sure why the preexisting bogo-productions have inconsistent > >>> indentation levels (looking at table_constraint_using_index) but > >>> that's not this patch's fault. > >> > >> Thanks for fixing that. > >> > >> I noticed that partition_bound_spec in the patch is missing hash > partition > >> bound syntax that was added after the original patch was written. Fixed > >> in the attached. > > > > I've pushed this with just a bit of re-ordering to match the order in > > which things show up in the synopsis. > > Thank you Stephen. > > Regards, > Amit > >
Re: non-bulk inserts and tuple routing
Fujita-san, Thank you for the review. On 2018/02/02 19:56, Etsuro Fujita wrote: > (2018/01/30 18:52), Etsuro Fujita wrote: >> (2018/01/30 18:39), Amit Langote wrote: >>> Will wait for your comments before sending a new version then. >> >> Ok, I'll post my comments as soon as possible. > > * ExecInitPartitionResultRelInfo is called from ExecFindPartition, but we > could call that another way; in ExecInsert/CopyFrom we call that after > ExecFindPartition if the partition chosen by ExecFindPartition has not > been initialized yet. Maybe either would be OK, but I like that because I > think that would not only better divide that labor but better fit into the > existing code in ExecInsert/CopyFrom IMO. I see no problem with that, so done that way. > * In ExecInitPartitionResultRelInfo: > + /* > + * Note that the entries in this list appear in no predetermined > + * order as result of initializing partition result rels as and when > + * they're needed. > + */ > + estate->es_leaf_result_relations = > + lappend(estate->es_leaf_result_relations, > + leaf_part_rri); > > Is it OK to put this within the "if (leaf_part_rri == NULL)" block? Good catch. I moved it outside the block. I was under the impression that leaf result relations that were reused from the mtstate->resultRelInfo arrary would have already been added to the list, but it seems they are not. > * In the same function: > + /* > + * Verify result relation is a valid target for INSERT. > + */ > + CheckValidResultRel(leaf_part_rri, CMD_INSERT); > > I think it would be better to leave the previous comments as-is here: > > /* > * Verify result relation is a valid target for an INSERT. An UPDATE > * of a partition-key becomes a DELETE+INSERT operation, so this > check > * is still required when the operation is CMD_UPDATE. > */ Oops, my bad. Didn't notice that I had ended up removing the part about UPDATE. > * ExecInitPartitionResultRelInfo does the work other than the > initialization of ResultRelInfo for the chosen partition (eg, create a > tuple conversion map to convert a tuple routed to the partition from the > parent's type to the partition's). So I'd propose to rename that function > to eg, ExecInitPartition. I went with ExevInitPartitionInfo. > * CopyFrom is modified so that it calls ExecSetupPartitionTupleRouting and > ExecFindPartition with a mostly-dummy ModifyTableState node. I'm not sure > that is a good idea. My concern about that is that might be something > like a headache in future development. OK, I removed those changes. > * The patch 0001 and 0002 are pretty small but can't be reviewed without > the patch 0003. The total size of the three patches aren't that large, so > I think it would be better to put those patches together into a single patch. As I said above, I got rid of 0001. Then, I merged the ExecFindPartition() refactoring patch 0002 into 0003. The code in tupconv_map_for_subplan() currently assumes that it can rely on all leaf partitions having been initialized. Since we're breaking that assumption with this proposal, that needed to be changed. So the patch contained some refactoring to make it not rely on that assumption. I carved that out into a separate patch which can be applied and tested before the main patch. > That's all for now. I'll continue to review the patches! Here is the updated version that contains two patches as described above. Thanks, Amit From 03ec63385251f31bdf95006258617d10eda94709 Mon Sep 17 00:00:00 2001 From: amit Date: Mon, 5 Feb 2018 13:38:05 +0900 Subject: [PATCH v24 1/2] Refactor partition tuple conversion maps handling code tupconv_map_for_subplan() currently assumes that it gets to use the Relation pointer for *all* leaf partitions. That is, both those that exist in mtstate->resultRelInfo array and those that don't and hence would be initialized by ExecSetupPartitionTupleRouting(). However, an upcoming patch will change ExecSetupPartitionTupleRouting such that leaf partitions' ResultRelInfo are no longer initialized there. So make it stop relying on the latter. --- src/backend/executor/execPartition.c | 87 ++ src/backend/executor/nodeModifyTable.c | 23 ++--- src/include/executor/execPartition.h | 2 + 3 files changed, 73 insertions(+), 39 deletions(-) diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 106a96d910..f2a920f4c3 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -373,40 +373,89 @@ ExecSetupChildParentMapForLeaf(PartitionTupleRouting *proute) } /* - * TupConvMapForLeaf -- Get the tuple conversion map for a given leaf partition - * index. + * ChildParentTupConvMap -- Return tuple conversion map to convert tuples of + * 'partrel'
Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently
On Fri, Feb 2, 2018 at 11:13 PM, Claudio Freire wrote:
> On Thu, Feb 1, 2018 at 9:34 PM, Masahiko Sawada wrote:
>> On Mon, Jan 29, 2018 at 11:31 PM, Claudio Freire
>> wrote:
>>> On Mon, Jan 29, 2018 at 4:12 AM, Masahiko Sawada
>>> wrote:
On Sat, Jul 29, 2017 at 9:42 AM, Claudio Freire
wrote:
> Introduce a tree pruning threshold to FreeSpaceMapVacuum that avoids
> recursing into branches that already contain enough free space, to
> avoid having to traverse the whole FSM and thus induce quadratic
> costs. Intermediate FSM vacuums are only supposed to make enough
> free space visible to avoid extension until the final (non-partial)
> FSM vacuum.
Hmm, I think this resolve a part of the issue. How about calling
AutoVacuumRequestWork() in PG_CATCH() if VACOPT_VACUUM is specified
and give the relid that we were vacuuming but could not complete as a
new autovacuum work-item? The new autovacuum work-item makes the
worker vacuum FSMs of the given relation and its indices.
>>>
>>> Well, I tried that in fact, as I mentioned in the OP.
>>>
>>> I abandoned it due to the conjunction of the 2 main blockers I found
>>> and mentioned there. In essence, those issues defeat the purpose of
>>> the patch (to get the free space visible ASAP).
>>>
>>> Don't forget, this is aimed at cases where autovacuum of a single
>>> relation takes a very long time. That is, very big relations. Maybe
>>> days, like in my case. A whole autovacuum cycle can take weeks, so
>>> delaying FSM vacuum that much is not good, and using work items still
>>> cause those delays, not to mention the segfaults.
>>
>> Yeah, I agree to vacuum fsm more frequently because it can prevent
>> table bloating from concurrent modifying. But considering the way to
>> prevent the table bloating from cancellation of autovacuum, I guess we
>> need more things. This proposal seems to provide us an ability that is
>> we "might be" able to prevent table bloating due to cancellation of
>> autovacuum. Since we can know that autovacuum is cancelled, I'd like
>> to have a way so that we can surely vacuum fsm even if vacuum is
>> cancelled. Thoughts?
>
> After autovacuum gets cancelled, the next time it wakes up it will
> retry vacuuming the cancelled relation. That's because a cancelled
> autovacuum doesn't update the last-vacuumed stats.
>
> So the timing between an autovacuum work item and the next retry for
> that relation is more or less an autovacuum nap time, except perhaps
> in the case where many vacuums get cancelled, and they have to be
> queued.
I think that's not true if there are multiple databases.
>
> That's why an initial FSM vacuum makes sense. It has a similar timing
> to the autovacuum work item, it has the benefit that it can be
> triggered manually (manual vacuum), and it's cheap and efficient.
>
>> Also the patch always vacuums fsm at beginning of the vacuum with a
>> threshold but it is useless if the table has been properly vacuumed. I
>> don't think it's good idea to add an additional step that "might be"
>> efficient, because current vacuum is already heavy.
>
> FSMs are several orders of magnitude smaller than the tables
> themselves. A TB-sized table I have here has a 95M FSM. If you add
> threshold skipping, that initial FSM vacuum *will* be efficient.
>
> By definition, the FSM will be less than 1/4000th of the table, so
> that initial FSM pass takes less than 1/4000th of the whole vacuum.
> Considerably less considering the simplicity of the task.
I agree the fsm is very smaller than heap and vacuum on fsm will not
be comparatively heavy but I'm afraid that the vacuum will get more
heavy in the future if we pile up such improvement that are small but
might not be efficient. For example, a feature for reporting the last
vacuum status has been proposed[1]. I wonder if we can use it to
determine whether we do the fsm vacuum at beginning of vacuum.
>> On detail of the patch,
>>
>> --- a/src/backend/storage/freespace/indexfsm.c
>> +++ b/src/backend/storage/freespace/indexfsm.c
>> @@ -70,5 +70,5 @@ RecordUsedIndexPage(Relation rel, BlockNumber usedBlock)
>> void
>> IndexFreeSpaceMapVacuum(Relation rel)
>> {
>> - FreeSpaceMapVacuum(rel);
>> + FreeSpaceMapVacuum(rel, 0);
>> }
>>
>> @@ -816,11 +820,19 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
>> bool *eof_p)
>> {
>> int child_avail;
>>
>> +/* Tree pruning for partial vacuums */
>> +if (threshold)
>> +{
>> + child_avail = fsm_get_avail(page, slot);
>> + if (child_avail >= threshold)
>> + continue;
>> +}
>>
>> Don't we skip all fsm pages if we set the threshold to 0?
>
> No, that doesn't skip anything if threshold is 0, since it doesn't get
> to the continue, which is the one skipping nodes.
Oops, yes you're right. Sorry for the noise.
[1]
https://www.postgresql.org/message-id/20171010.192616.108347483.horiguchi.kyotaro%40lab.ntt.c
Re: constraint exclusion and nulls in IN (..) clause
On 2018/02/05 13:20, Ashutosh Bapat wrote: > On Thu, Feb 1, 2018 at 2:23 PM, Amit Langote > wrote: >> >> Yeah, the patch in its current form is wrong, because it will give wrong >> answers if the operator being used in a SAOP is non-strict. I modified >> the patch to consider operator strictness before doing anything with nulls. > > That's fine, but I am not sure whether this fits constraint > exclusion's charter. Please add this patch to the next commitfest so > that we can have a better opinion. OK, done. Thanks, Amit
Re: constraint exclusion and nulls in IN (..) clause
On Thu, Feb 1, 2018 at 2:23 PM, Amit Langote wrote: > > Yeah, the patch in its current form is wrong, because it will give wrong > answers if the operator being used in a SAOP is non-strict. I modified > the patch to consider operator strictness before doing anything with nulls. That's fine, but I am not sure whether this fits constraint exclusion's charter. Please add this patch to the next commitfest so that we can have a better opinion. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
RE: Temporary tables prevent autovacuum, leading to XID wraparound
From: Robert Haas [mailto:robertmh...@gmail.com] > Temporary tables contain XIDs, so they need to be vacuumed for XID > wraparound. Otherwise, queries against those tables by the session > that created them could yield wrong answers. However, autovacuum > can't perform that vacuuming; it would have to be done by the session. > I think we should consider having backends try to remove their > temporary schema on startup; then, if a temp table in a backend is old > enough that it's due for vacuum for wraparound, have autovacuum kill > the connection. The former is necessary to prevent sessions from > being killed on account of temp tables they "inherited" from a backend > that didn't exit cleanly. The attached patch does the former. The small change in autovacuum.c is mainly for autovac launcher and background workers which don't connect to a database. I'll add this to the next CF. I'd like this to be back-patched. I didn't do the latter, because killing the connection seemed a bit overkill. If we're going to do it, then we should also kill the connection which is preventing vacuum regardless of whether it has temporary tables in its session. Regards Takayuki Tsunakawa reset_temp_schema_on_connect.patch Description: reset_temp_schema_on_connect.patch
Re: [HACKERS][PATCH] Applying PMDK to WAL operations for persistent memory
>On Tue, Jan 30, 2018 at 3:37 AM, Yoshimi Ichiyanagi > wrote: >> Oracle and Microsoft SQL Server suported PMEM [1][2]. >> I think it is not too early for PostgreSQL to support PMEM. > >I agree; it's good to have the option available for those who have >access to the hardware. > >If you haven't added your patch to the next CommitFest, please do so. Thank you for your time. I added my patches to the CommitFest 2018-3. https://commitfest.postgresql.org/17/1485/ Oh by the way, we submitted this proposal(Introducing PMDK into PostgreSQL) to PGcon2018. If our proposal is accepted and you have time, please listen to our presentation. -- Yoshimi Ichiyanagi Mailto : ichiyanagi.yosh...@lab.ntt.co.jp
RE: [bug fix] ECPG: freeing memory for pgtypes crashes on Windows
From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com] > +#ifndef PGTYPES_FREE > +#define PGTYPES_FREE > + extern void PGTYPES_free(void *ptr); > +#endif > > It seems quite strange to repeat this in pgtypes_date.h, pgtypes_interval.h > and pgtypes_numeric.h. I guess you might not want to introduce a new common > header file so that his can be back-patched more easily? Not sure if there > is a project policy about that, but it seems unfortunate to introduce > maintenance burden by duplicating this. Your guess is correct. I wanted to avoid the duplication, but there was no good place to put this without requiring existing applications to change their #include directives. > + PGTYPES_free()/ instead of > free(). > > The "/" needs to move inside then closing tag. Thanks, fixed. Regards Takayuki Tsunakawa pgtypes_freemem_v2.patch Description: pgtypes_freemem_v2.patch
Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis
On 2018/02/02 19:41, Stephen Frost wrote: > * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote: >> On 2018/01/23 8:57, Thomas Munro wrote: >>> Here's an update to move the new stuff to the correct side of the >>> closing "". Builds for me, and the typesetting looks OK. >>> I'm not sure why the preexisting bogo-productions have inconsistent >>> indentation levels (looking at table_constraint_using_index) but >>> that's not this patch's fault. >> >> Thanks for fixing that. >> >> I noticed that partition_bound_spec in the patch is missing hash partition >> bound syntax that was added after the original patch was written. Fixed >> in the attached. > > I've pushed this with just a bit of re-ordering to match the order in > which things show up in the synopsis. Thank you Stephen. Regards, Amit
Re: [bug fix] ECPG: freeing memory for pgtypes crashes on Windows
On Fri, Feb 2, 2018 at 3:47 PM, Tsunakawa, Takayuki wrote: > The fix is to add PGTYPES_free() in libpgtypes.dll, just like libpq has > PQfreemem() described here: +#ifndef PGTYPES_FREE +#define PGTYPES_FREE + extern void PGTYPES_free(void *ptr); +#endif It seems quite strange to repeat this in pgtypes_date.h, pgtypes_interval.h and pgtypes_numeric.h. I guess you might not want to introduce a new common header file so that his can be back-patched more easily? Not sure if there is a project policy about that, but it seems unfortunate to introduce maintenance burden by duplicating this. + PGTYPES_free()/ instead of free(). The "/" needs to move inside then closing tag. -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] MERGE SQL Statement for PG11
On Sun, Feb 4, 2018 at 12:42 AM, Simon Riggs wrote: > I'm happy to quote your words. > > "I've acknowledged that the standard has something to > say on this that supports your position, which has real weight." > > https://www.postgresql.org/message-id/CAH2-WzkAjSN1H-ym-sSDh%2B6EJWmEhyHdDStzXDB%2BFxt1hcKEgg%40mail.gmail.com Immediately afterwards, in that same e-mail, I go on to say: "I'm not asking about WHEN AND here (that was my last question) [Simon quoted my last response to said question]. I'm asking about a subselect that appears in the targetlist." Even if you are right to take "I've acknowledged that the standard has something to say on this that supports your position" as a blanket endorsement of disallowing subselects in all 3 places, I still don't see why this is worth even talking about. That would mean that I said something on January 29th that I subsequently withdrew on February 1st. -- Peter Geoghegan
Re: ALTER TABLE ADD COLUMN fast default
On Mon, Feb 5, 2018 at 7:19 AM, Thomas Munro wrote: > On Fri, Jan 26, 2018 at 1:23 PM, Andrew Dunstan > wrote: >> Yeah, thanks. revised patch attached > > FYI the identity regression test started failing recently with this > patch applied (maybe due to commit > 533c5d8bddf0feb1785b3da17c0d17feeaac76d8?) > Thanks. Probably the same bug Tomas Vondra found a few days ago. I'm on it. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: WIP Patch: Precalculate stable functions, infrastructure v1
On Thu, Feb 1, 2018 at 6:01 PM, Marina Polyakova wrote: > This is the 8-th version of the patch for the precalculation of stable or > immutable functions, stable or immutable operators and other nonvolatile > expressions. This is a try to fix the most problems (I'm sorry, it took some > time..) that Tom Lane and Andres Freund mentioned in [1], [2] and [3]. It is > based on the top of master, and on my computer make check-world passes. And > I'll continue work on it. Hi Marina, FYI I saw a repeatable crash in the contrib regression tests when running make check-world with this patch applied. test hstore_plperl... FAILED (test process exited with exit code 2) test hstore_plperlu ... FAILED (test process exited with exit code 2) test create_transform ... FAILED I'm not sure why it passes for you but fails here, but we can see from the backtrace[1] that ExecInitExprRec is receiving a null node pointer on this Ubuntu Trusty GCC 4.8 amd64 system. [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/337255374 -- Thomas Munro http://www.enterprisedb.com
Re: ALTER TABLE ADD COLUMN fast default
On Fri, Jan 26, 2018 at 1:23 PM, Andrew Dunstan wrote: > Yeah, thanks. revised patch attached FYI the identity regression test started failing recently with this patch applied (maybe due to commit 533c5d8bddf0feb1785b3da17c0d17feeaac76d8?) *** /home/travis/build/postgresql-cfbot/postgresql/src/test/regress/expected/identity.out 2018-02-04 00:56:12.146303616 + --- /home/travis/build/postgresql-cfbot/postgresql/src/test/regress/results/identity.out 2018-02-04 01:05:55.217932032 + *** *** 257,268 INSERT INTO itest13 VALUES (1), (2), (3); -- add column to populated table ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY; SELECT * FROM itest13; ! a | b | c ! ---+---+--- ! 1 | 1 | 1 ! 2 | 2 | 2 ! 3 | 3 | 3 (3 rows) -- various ALTER COLUMN tests --- 257,269 INSERT INTO itest13 VALUES (1), (2), (3); -- add column to populated table ALTER TABLE itest13 ADD COLUMN c int GENERATED BY DEFAULT AS IDENTITY; + ERROR: column "c" contains null values SELECT * FROM itest13; ! a | b ! ---+--- ! 1 | 1 ! 2 | 2 ! 3 | 3 (3 rows) -- various ALTER COLUMN tests -- Thomas Munro http://www.enterprisedb.com
Re: MCV lists for highly skewed distributions
On 2/2/18, Dean Rasheed wrote: > I think it would be better to try to come up with an alternative > algorithm that has a better theoretical basis, and then test that to > see how it holds up in practice. > > With that in mind, attached is a patch based on the idea of setting a > bound on the relative standard error on the sample frequency I did the same basic eyeball testing I did on earlier patches, and this is the best implementation so far. I've attached some typical pg_stats contents for HEAD and this patch. More rigorous testing, including of planner estimates on real data, is still needed of course, but I think this is definitely in the right direction. -John Naylor test_mcvstats_v1.sql Description: application/sql
Re: [HACKERS] MERGE SQL Statement for PG11
Simon Riggs writes: > It will likely take some time to work through these and the current > work items but will fix. > > Do you have the DDL so we can recreate these easily? Attached are testcases that trigger the assertions when run against an empty database instead of the one left behind by make installcheck. The "unrecognized node type" one appears to be a known issue according to the wiki, so I didn't bother doing the work for this one as well. regards, Andreas TRAP-node-type-T_SubLink-prepunion.sql Description: application/sql TRAP-node-type-T_SubLink-clauses.sql Description: application/sql TRAP-node-type-T_Query.sql Description: application/sql
Support for ECDSA & ed25519 digital signatures in pgcrypto?
I recently had to build ed25519 digital signature validation in PostgreSQL. Since pgcrypto doesn't support these methods, I had to look into PL/Python and PL/v8 based implementations. The experience turned out to be very poor (documented here: https://gist.github.com/nileshtrivedi /7cd622d4d521986593bff81bfa1e5893 I think OpenSSL already supports these encryption methods and it would be great to have them supported within pgcrypto - especially with the advent of distributed systems like IPFS, public blockchains like BitCoin, Ethereum. Elliptic curve cryptography has some major advantages over RSA: for both security and usability. Some are listed here: https://ed25519.cr.yp.to/ Is somebody working on this? I'm not a C programmer but if needed, I can look into implementing this.
Re: [HACKERS] MERGE SQL Statement for PG11
On 3 February 2018 at 23:17, Peter Geoghegan wrote: > There are 3 specific issues on query structure, that together paint a > picture about what you're not doing in the optimizer: > > 1. Whether or not subselects in the UPDATE targetlist are supported. > > 2. Whether or not subselects in the WHEN ... AND quals support subselects. > > 3. Whether or not subselects are possible within the main ON () join. > > I gave a lukewarm endorsement of not supporting #3, was unsure with > #2, and was very clear on #1 as soon as I saw the restriction: UPDATE > targetlist in a MERGE are *not* special, and so must support > subselects, just like ON CONFLICT DO UPDATE, for example. All three of the above give errors in the current patch, as we already discussed for (1) and (2). I've added these to the tests so we can track support for them explicitly. The current patch runs one query then executes the quals post-execution as we do for check constraints and RLS. Changes would be required to support subselects. Changes to support sub-selects don't invalidate what is there now in the current patch with regard to query representation or optimization. So support of those extra features can be added later if we choose. Frankly, whatever we do now, I'm sure we will discover cases that need further optimization, just as we have done with RLS and Partitioning, so the query representation was likely to change over time anyway. Whatever we decide for concurrent behavior will affect how we support them. We can't move forwards on them until we have that nailed down. I could give a longer technical commentary but I will be unavailable now for some time, so unable to give further discussion. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] MERGE SQL Statement for PG11
On 4 February 2018 at 06:32, Amit Kapila wrote: > On Wed, Jan 31, 2018 at 11:37 PM, Peter Geoghegan wrote: >> On Wed, Jan 31, 2018 at 7:17 AM, Robert Haas wrote: >>> I don't fully grok merge but suppose you have: >>> >>> WHEN MATCHED AND a = 0 THEN UPDATE ... >>> WHEN MATCHED AND a = 1 THEN UPDATE ... >>> WHEN NOT MATCHED THEN INSERT ... >>> >>> Suppose you match a tuple with a = 0 but, upon trying to update it, >>> find that it's been updated to a = 1. It seems like there are a few >>> possible behaviors: >>> >>> 1. Throw an error! I guess this is what the patch does now. >> >> Right. >> >>> 2. Do absolutely nothing. I think this is what would happen with an >>> ordinary UPDATE; the tuple fails the EPQ recheck and so is not >>> updated, but that doesn't trigger anything else. >> >> I think #2 is fine if you're talking about join quals. Which, of >> course, you're not. These WHEN quals really do feel like >> tuple-at-a-time procedural code, more than set-orientated quals (if >> that wasn't true, we'd have to allow cardinality violations, which we >> at least try to avoid). Simon said something like "the SQL standard >> requires that WHEN quals be evaluated first" at one point, which makes >> sense to me. >> > > It is not clear to me what is exactly your concern if we try to follow > #2? To me, #2 seems like a natural choice. At first, but it gives an anomaly so is not a good choice. The patch does behavior #5, it rechecks the conditions with the latest row. Otherwise WHEN MATCHED AND a=0 THEN UPDATE SET b=0 WHEN MATCHED AND a=1 THEN UPDATE SET b=1 would result in (a=1, b=0) in case of concurrent updates, which the user clearly doesn't want. The evaluation of the WHEN qual must occur prior to the update, which will still be true in #5. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] MERGE SQL Statement for PG11
On 3 February 2018 at 23:17, Peter Geoghegan wrote: > On Sat, Feb 3, 2018 at 2:15 PM, Simon Riggs wrote: >>> I started looking at SQL Server's MERGE to verify that it also does >>> not impose any restrictions on subselects in a MERGE UPDATE's >>> targetlist, just like Oracle. Unsurprisingly, it does not. More >>> surprisingly, I noticed that it also doesn't seem to impose >>> restrictions on what can appear in WHEN ... AND quals. >> >> You earlier agreed that subselects were not part of the Standard. > > You know that I didn't say that, Simon. I'm happy to quote your words. "I've acknowledged that the standard has something to say on this that supports your position, which has real weight." https://www.postgresql.org/message-id/CAH2-WzkAjSN1H-ym-sSDh%2B6EJWmEhyHdDStzXDB%2BFxt1hcKEgg%40mail.gmail.com -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
