date:20170118

Re: [HACKERS] Patch to implement pg_current_logfile() function

2017-01-18 Thread Michael Paquier

On Thu, Jan 19, 2017 at 7:27 AM, Alvaro Herrera
 wrote:
> I thought this part was odd -- I mean, why is SysLogger_Start() being
> called if the collector is not enabled?  Turns out we do it and return
> early if not enabled.  But not in all cases -- there is one callsite in
> postmaster.c that avoids the call if the collector is disabled.  That
> needs to be changed if we want this to work reliably.

Indeed. And actually it is fine to remove the call to FreeFile() in
the error code path of pg_current_logfile().

> I don't think the "abstract names" stuff is an improvement (just look at
> the quoting mess in ConfigureNamesString).  I think we should do without
> that; at least as part of this patch.  If you think there's code that
> can get better because of the idea, let's see it.

Agreed.
-- 
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] pg_hba_file_settings view patch

2017-01-18 Thread Michael Paquier

On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
 wrote:
> Added the cleanup mechanism. But the tokenize_file() function call
> present in many places, But in one flow still it is possible to have
> file descriptor leak because of pg_hba_rules view. Because of this
> reason, added the cleanup everywhere.

Oops, sorry. Actually you don't need that. AllocateFile() registers
the fd opened with the sub-transactions it is involved with... So if
there is an ERROR nothing leaks.
-- 
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] pg_hba_file_settings view patch

2017-01-18 Thread Haribabu Kommi

On Thu, Jan 19, 2017 at 4:08 PM, Michael Paquier 
wrote:

> On Wed, Jan 18, 2017 at 4:11 PM, Haribabu Kommi
>  wrote:
> > updated patch attached.
>
> Thanks for the new version.
>
> > Added tap tests patch also attached.
>
> This begins to look really nice. I am having fun torturing it :)
>

Thanks for the review.

Here are I think my last comments:
>
> +   linecxt = tokenize_file(HbaFileName, file, &hba_lines,
> &hba_line_nums, &hba_raw_lines);
> +   FreeFile(file);
> tokenize_file can leave on ERROR, in which case the file descriptor
> would leak. You much likely need a
> PG_END_ENSURE_ERROR_CLEANUP/PG_ENSURE_ERROR_CLEANUP block here with a
> callback to FreeFile() if an error is caught.
>

Added the cleanup mechanism. But the tokenize_file() function call
present in many places, But in one flow still it is possible to have
file descriptor leak because of pg_hba_rules view. Because of this
reason, added the cleanup everywhere.


> + 
> +  ADDRESS specifies the set of hosts the record matches.
> +  It can be a host name, or it is made up of an IP address
> +  or keywords such as (all,
> +  samehost and samenet).
> + 
> Why is that upper-case?
>

Corrected.

+ 
> +  If not null, an error message indicating why this
> +  rule could not be loaded
> + 
> Need a dot here, that's a sentence.
>

updated.

src/test/regress/expected/rules.out needs to be refreshed, regression
> tests are failing.
>

Corrected.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia


pg_hba_rules_9.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] PoC: Grouped base relation

2017-01-18 Thread Tomas Vondra


On 01/19/2017 04:09 AM, Ashutosh Bapat wrote:

On Thu, Jan 19, 2017 at 12:02 AM, Robert Haas  wrote:

On Tue, Jan 17, 2017 at 11:33 PM, Ashutosh Bapat


Also, the point is naming that kind of function as aggtransmultifn
would mean that it's always supposed to multiply, which isn't true for
all aggregates.


TransValue * integer = newTransValue is well-defined for any
aggregate.  It's the result of aggregating that TransValue with itself
a number of times defined by the integer.  And that might well be
significantly faster than using aggcombinefn many times.  On the other
hand, how many queries just sit there are re-aggregate the same
TransValues over and over again?  I am having trouble wrapping my head
around that part of this.


Not all aggregates have TransValue * integer = newTransValue
behaviour. Example is array_agg() or string_agg() has "TransValue
concatenated integer time" behaviour. Or an aggregate "multiplying"
values across rows will have TransValue (raised to) integer behaviour.
Labelling all of those as "multi" doesn't look good.



All aggregates that have (or can have) a combine function have it, 
because in the worst case you can simply implement it by calling the 
combine function repeatedly.


Also, if you treat the combine function as "+" then the "multiply" 
function is exactly what "*" is expected to do. So I find the naming 
quite appropriate, actually.


But I think naming of the function is not the most important aspect of 
the patch, I believe. In the worst case, we can do s/multi/whatever/ 
sometime later.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Causal reads take II

2017-01-18 Thread Ants Aasma

On Tue, Jan 3, 2017 at 3:43 AM, Thomas Munro
 wrote:
> Here is a new version of my "causal reads" patch (see the earlier
> thread from the 9.6 development cycle[1]), which provides a way to
> avoid stale reads when load balancing with streaming replication.

Thanks for working on this. It will let us do something a lot of
people have been asking for.

> Long term, I think it would be pretty cool if we could develop a set
> of features that give you distributed sequential consistency on top of
> streaming replication.  Something like (this | causality-tokens) +
> SERIALIZABLE-DEFERRABLE-on-standbys[3] +
> distributed-dirty-read-prevention[4].

Is it necessary that causal writes wait for replication before making
the transaction visible on the master? I'm asking because the per tx
variable wait time between logging commit record and making
transaction visible makes it really hard to provide matching
visibility order on master and standby. In CSN based snapshot
discussions we came to the conclusion that to make standby visibility
order match master while still allowing for async transactions to
become visible before they are durable we need to make the commit
sequence a vector clock and transmit extra visibility ordering
information to standby's. Having one more level of delay between wal
logging of commit and making it visible would make the problem even
worse.

One other thing that might be an issue for some users is that this
patch only ensures that clients observe forwards progress of database
state after a writing transaction. With two consecutive read only
transactions that go to different servers a client could still observe
database state going backwards. It seems that fixing that would
require either keeping some per client state or a global agreement on
what snapshots are safe to provide, both of which you tried to avoid
for this feature.

Regards,
Ants Aasma

-- 
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] Transactions involving multiple postgres foreign servers

2017-01-18 Thread vinayak



On 2017/01/16 17:35, Masahiko Sawada wrote:

On Fri, Jan 13, 2017 at 3:48 PM, Masahiko Sawada  wrote:

On Fri, Jan 13, 2017 at 3:20 PM, Ashutosh Bapat
 wrote:

Long time passed since original patch proposed by Ashutosh, so I
explain again about current design and functionality of this feature.
If you have any question, please feel free to ask.

Thanks for the summary.


Parameters
==

[ snip ]


Cluster-wide atomic commit
===
Since the distributed transaction commit on foreign servers are
executed independently, the transaction that modified data on the
multiple foreign servers is not ensured that transaction did either
all of them commit or all of them rollback. The patch adds the
functionality that guarantees distributed transaction did either
commit or rollback on all foreign servers. IOW the goal of this patch
is achieving the cluster-wide atomic commit across foreign server that
is capable two phase commit protocol.

In [1], I proposed that we solve the problem of supporting PREPARED
transactions involving foreign servers and in subsequent mail Vinayak
agreed to that. But this goal has wider scope than that proposal. I am
fine widening the scope, but then it would again lead to the same
discussion we had about the big picture. May be you want to share
design (or point out the parts of this design that will help) for
solving smaller problem and tone down the patch for the same.


Sorry for confuse you. I'm still focusing on solving only that
problem. What I was trying to say is that I think that supporting
PREPARED transaction involving foreign server is the means, not the
end. So once we supports PREPARED transaction involving foreign
servers we can achieve cluster-wide atomic commit in a sense.


Attached updated patches. I fixed some bugs and add 003 patch that
adds TAP test for foreign transaction.
003 patch depends 000 and 001 patch.

Please give me feedback.


I have tested prepared transactions with foreign servers but after 
preparing the transaction

the following error occur infinitely.
Test:
=
=#BEGIN;
=#INSERT INTO ft1_lt VALUES (10);
=#INSERT INTO ft2_lt VALUES (20);
=#PREPARE TRANSACTION 'prep_xact_with_fdw';

2017-01-18 15:09:48.378 JST [4312] ERROR:  function pg_fdw_resolve() 
does not exist at character 8
2017-01-18 15:09:48.378 JST [4312] HINT:  No function matches the given 
name and argument types. You might need to add explicit type casts.

2017-01-18 15:09:48.378 JST [4312] QUERY:  SELECT pg_fdw_resolve()
2017-01-18 15:09:48.378 JST [29224] LOG:  worker process: foreign 
transaction resolver (dbid 13119) (PID 4312) exited with exit code 1

.

If we check the status on another session then it showing the status as 
prepared.

=# select * from pg_fdw_xacts;
 dbid  | transaction | serverid | userid |  status  | identifier
---+-+--++--+ 

 13119 |1688 |16388 | 10 | prepared | 
px_2102366504_16388_10
 13119 |1688 |16389 | 10 | prepared | 
px_749056984_16389_10

(2 rows)

I think this is a bug.

Regards,
Vinayak Pokale
NTT Open Source Software Center



--
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] Add pgstathashindex() to get hash index table statistics.

2017-01-18 Thread Ashutosh Sharma

> However, I've some minor comments on the patch:
>
> +/*
> + * HASH_ALLOCATABLE_PAGE_SZ represents allocatable
> + * space (pd_upper - pd_lower) on a hash page.
> + */
> +#define HASH_ALLOCATABLE_PAGE_SZ \
> +   BLCKSZ - \
> +   (SizeOfPageHeaderData + sizeof(HashPageOpaqueData))
> My suggestion will be not to write "(pd_upper - pd_lower)" in the
> comment. You may write allocatable space on a empty hash page.

Accepted. Have changed the comment accordingly.

>
> +   buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno,
> RBM_NORMAL, NULL);
> Use BAS_BULKREAD strategy to read the buffer.
>

okay, corrected. Please check the attached v3 patch with corrections.

With Regards,
Ashutosh Sharma
EnterpriseDB: http://www.enterprisedb.com


0001-Add-pgstathashindex-to-pgstattuple-extension-v3.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Review: GIN non-intrusive vacuum of posting tree

2017-01-18 Thread Jeff Davis

https://www.postgresql.org/message-id/CAJEAwVE4UAmm8fr%2BNW8XTnKV6M--ACoNhL3ES8yoKL2sKhbaiw%40mail.gmail.com

Let me re-summarize what's been done here to make sure I understand:

Each key in GIN has its own posting tree, which is itself a btree,
holding all of the tuples that contain that key. This posting tree is
really just a set of tuples, and searches always scan an entire
posting tree (because all the tuples contain the key, so all are a
potential match).

Currently, the cleanup process requires blocking all reads and writes
in the posting list. I assume this is only a problem when a key is
common enough that its posting list is quite large (how large?).

This patch makes a couple changes to improve concurrency:

1. Vacuum leaves first, without removing any pages. Instead, just keep
track of pages which are completely empty (more generally, subtrees
that contain empty pages).

2. Free the empty pages in those subtrees. That requires blocking
reads and writes to that subtree, but not the entire posting list.
Blocking writes is easy: acquiring a cleanup lock on the root of any
subtree always blocks any writes to that subtree, because the write
keeps a pin on all pages in that path. Blocking reads is accomplished
by locking the page to be deleted, then locking/unlocking the page one
left of that (which may be outside the subtree?).

Maybe a basic question, but why is the posting list a btree at all,
and not, say a doubly-linked list?

Review of the code itself:

* Nice and simple, with a net line count of only +45.
* Remove commented-out code.
* In the README, "leaf-to-right" should be left-to-right; and "takinf"
should be "taking".
* When you say the leftmost page is never deleted; do you mean the
leftmost of the entire posting tree, or leftmost of the subtree that
you are removing pages from?
* In order to keep out concurrent reads, you need to lock/unlock the
left page while holding exclusive lock on the page being deleted, but
I didn't see how that happens exactly in the code. Where does that
happen?

Regards,
Jeff Davis

--
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] Password identifiers, protocol aging and SCRAM protocol

2017-01-18 Thread Noah Misch

On Wed, Jan 18, 2017 at 02:30:38PM +0900, Michael Paquier wrote:
> On Wed, Jan 18, 2017 at 2:23 PM, Noah Misch  wrote:
> > The latest versions document this precisely, but I agree with Peter's 
> > concern
> > about plain "scram".  Suppose it's 2025 and PostgreSQL support SASL 
> > mechanisms
> > OAUTHBEARER, SCRAM-SHA-256, SCRAM-SHA-256-PLUS, and SCRAM-SHA3-512.  What
> > should the pg_hba.conf options look like at that time?  I don't think 
> > having a
> > single "scram" option fits in such a world.
> 
> Sure.
> 
> > I see two strategies that fit:
> >
> > 1. Single "sasl" option, with a GUC, similar to ssl_ciphers, controlling the
> >mechanisms to offer.
> > 2. Separate options "scram_sha_256", "scram_sha3_512", "oauthbearer", etc.
> 
> Or we could have a sasl option, with a mandatory array of mechanisms
> to define one or more items, so method entries in pg_hba.conf would
> look llke that:
> sasl mechanism=scram_sha_256,scram_sha3_512

I like that.


-- 
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 to implement pg_current_logfile() function

2017-01-18 Thread Alvaro Herrera

Karl O. Pinc wrote:

> @@ -511,10 +519,16 @@ int
>  SysLogger_Start(void)
>  {
>   pid_t   sysloggerPid;
> - char   *filename;
>  
> + /*
> +  * Logging collector is not enabled. We don't know where messages are
> +  * logged.  Remove outdated file holding the current log filenames.
> +  */
>   if (!Logging_collector)
> + {
> + unlink(LOG_METAINFO_DATAFILE);
>   return 0;
> + }

I thought this part was odd -- I mean, why is SysLogger_Start() being
called if the collector is not enabled?  Turns out we do it and return
early if not enabled.  But not in all cases -- there is one callsite in
postmaster.c that avoids the call if the collector is disabled.  That
needs to be changed if we want this to work reliably.

I don't think the "abstract names" stuff is an improvement (just look at
the quoting mess in ConfigureNamesString).  I think we should do without
that; at least as part of this patch.  If you think there's code that
can get better because of the idea, let's see it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Declarative partitioning - another take

2017-01-18 Thread Amit Langote

On 2017/01/19 5:29, Robert Haas wrote:
> On Wed, Jan 18, 2017 at 3:12 PM, Robert Haas  wrote:
>> On Tue, Jan 10, 2017 at 6:06 AM, Amit Langote
>>  wrote:
>>> [ updated patches ]
>>
>> I committed 0004 and also fixed the related regression test not to
>> rely on DROP .. CASCADE, which isn't always stable.  The remainder of

Thanks!

>> this patch set needs a rebase, and perhaps you could also fold in
>> other pending partitioning fixes so I have everything to look at it in
>> one place.
> 
> Just to be a little more clear, I don't mind multiple threads each
> with a patch or patch set so much, but multiple patch sets on the same
> thread gets hard for me to track.  Sorry for the inconvenience.

OK, I agree that having multiple patch sets on the same thread is cumbersome.

So, here are all the patches I posted to date (and one new at the bottom)
for reported and unreported bugs, excluding the one involving
BulkInsertState for which you replied in a new thread.

I'll describe the attached patches in brief:

0001-Fix-a-bug-of-insertion-into-an-internal-partition.patch

Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.

0002-Set-ecxt_scantuple-correctly-for-tuple-routing.patch

In 2ac3ef7a01df859c62d0a02333b646d65eaec5ff, we changed things so that
it's possible for a different TupleTableSlot to be used for partitioned
tables at successively lower levels.  If we do end up changing the slot
from the original, we must update ecxt_scantuple to point to the new one
for partition key of the tuple to be computed correctly.

Last posted here:
https://www.postgresql.org/message-id/99fbfad6-b89b-9427-a6ca-197aad98c48e%40lab.ntt.co.jp

0003-Fix-RETURNING-to-work-correctly-after-tuple-routing.patch

In ExecInsert(), do not switch back to the root partitioned table
ResultRelInfo until after we finish ExecProcessReturning(), so that
RETURNING projection is done using the partition's descriptor.  For
the projection to work correctly, we must initialize the same for
each leaf partition during ModifyTableState initialization.

0004-Fix-some-issues-with-views-and-partitioned-tables.patch

Automatically updatable views failed to handle partitioned tables.
Once that's fixed, WITH CHECK OPTIONS wouldn't work correctly without
the WCO expressions having been suitably converted for each partition
(think applying map_partition_varattnos to Vars in the WCO expressions
just like with partition constraint expressions).

0005-Fix-some-wrong-thinking-in-check_new_partition_bound.patch

Because a given range bound in the PartitionBoundInfo.datums array
is sometimes a range lower bound and upper bound at other times, we
must be careful when assuming which, especially when interpreting
the result of partition_bound_bsearch which returns the index of the
greatest bound that is less than or equal to probe.  Due to an error
in thinking about the same, the relevant code in
check_new_partition_bound() caused invalid partition (index = -1)
to be chosen as the partition being overlapped.

Last posted here:
https://www.postgresql.org/message-id/603acb8b-5dec-31e8-29b0-609a68aac50f%40lab.ntt.co.jp

0006-Avoid-tuple-coversion-in-common-partitioning-cases.patch

Currently, the tuple conversion is performed after a tuple is routed,
even if the attributes of a target leaf partition map one-to-one with
those of the root table, which is wasteful.  Avoid that by making
convert_tuples_by_name() return a NULL map for such cases.

0007-Avoid-code-duplication-in-map_partition_varattnos.patch

Code to map attribute numbers in map_partition_varattnos() duplicates
what convert_tuples_by_name_map() does.  Avoid that as pointed out by
Álvaro Herrera.

Last posted here:
https://www.postgresql.org/message-id/9ce97382-54c8-deb3-9ee9-a2ec271d866b%40lab.ntt.co.jp

0008-Avoid-DROP-TABLE-.-CASCADE-in-more-partitioning-test.patch

This is the new one.  There were quite a few commits recently to fix the
breakage in regression tests due to not using ORDER BY in queries on
system catalogs and using DROP TABLE ... CASCADE.  There were still some
instances of the latter in create_table.sql and alter_table.sql.

Thanks,
Amit
>From 37c861d5eae1c8f11b3fae93cb209da262a13fbc Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 13 Dec 2016 15:07:41 +0900
Subject: [PATCH 1/8] Fix a bug of insertion into an internal partition.

Since implicit partition constraints are not inherited, an internal
partition's constraint was not being enforced when targeted directly.
So, include such constraint when setting up leaf partition result
relations for tuple-routing.

Reported by: n/a
Patch by: Amit Langote
Reports: n/a
---
 src/backend/commands/copy.c  |  1 -
 src/backend/commands/tablecmds.c |  1 -
 src/backend/executor/execMain.c  | 42 
 src/include/executor/executor.h

Re: [HACKERS] pg_hba_file_settings view patch

2017-01-18 Thread Michael Paquier

On Wed, Jan 18, 2017 at 4:11 PM, Haribabu Kommi
 wrote:
> updated patch attached.

Thanks for the new version.

> Added tap tests patch also attached.

This begins to look really nice. I am having fun torturing it :)

Here are I think my last comments:

+   linecxt = tokenize_file(HbaFileName, file, &hba_lines,
&hba_line_nums, &hba_raw_lines);
+   FreeFile(file);
tokenize_file can leave on ERROR, in which case the file descriptor
would leak. You much likely need a
PG_END_ENSURE_ERROR_CLEANUP/PG_ENSURE_ERROR_CLEANUP block here with a
callback to FreeFile() if an error is caught.

+ 
+  ADDRESS specifies the set of hosts the record matches.
+  It can be a host name, or it is made up of an IP address
+  or keywords such as (all,
+  samehost and samenet).
+ 
Why is that upper-case?

+ 
+  If not null, an error message indicating why this
+  rule could not be loaded
+ 
Need a dot here, that's a sentence.

src/test/regress/expected/rules.out needs to be refreshed, regression
tests are failing.
-- 
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] postgres_fdw bug in 9.6

2017-01-18 Thread Ashutosh Bapat

On Thu, Jan 19, 2017 at 2:14 AM, Robert Haas  wrote:
> On Fri, Jan 13, 2017 at 6:22 AM, Etsuro Fujita
>  wrote:
>> My biggest concern about GetExistingLocalJoinPath is that might not be
>> extendable to the case of foreign-join paths with parameterization; in which
>> case, fdw_outerpath for a given foreign-join path would need to have the
>> same parameterization as the foreign-join path, but there might not be any
>> existing paths with the same parameterization in the path list.
>
> I agree that this is a problem.

Effectively, it means that foreign join path creation will have a
parameterization different (per add_path()) from any local join
produced. But why would it be so? The parameterization bubbles up from
the base relation. The process for creating parameterized local and
foreign paths for a base relation is same. Thus we will have same
parameterizations considered for foreign as well as local joins. Those
different parameterizations will be retained add_path(), so we should
find one there or should be able to expand an existing
parameterization by reparameterization.

Even if such a case exists, the same problem exists while searching
paths from the joining relations. If the join doesn't have a local
path with required parameterization, so would be the joining
relations, parameterized paths from which are used to build
parameterized paths for join.

>
>> You might
>> think we could get the fdw_outerpath by getting an existing path with no
>> parameterization as in GetExistingLocalJoinPath and then modifying the
>> path's param_info to match the parameterization of the foreign-join path.  I
>> don't know that really works, but that might be inefficient.
>
> I am not sure about this.
>
>> What I have in mind to support foreign-join paths with parameterization for
>> postgres_fdw like this: (1) generate parameterized paths from any joinable
>> combination of the outer/inner cheapest-parameterized paths that have pushed
>> down the outer/inner relation to the remote server, in a similar way as
>> postgresGetForeignJoinPaths creates unparameterized paths, and (2) create
>> fdw_outerpath for each parameterized path from the outer/inner paths used to
>> generate the parameterized path, by create_nestloop_path (or,
>> create_hashjoin_path or create_mergejoin_path if full join), so that the
>> resulting fdw_outerpath has the same parameterization as the paramterized
>> path.  This would probably work and might be more efficient.  And the patch
>> I proposed would be easily extended to this, by replacing the outer/inner
>> cheapest-total paths with the outer/inner cheapest-parameterized paths.
>> Attached is the latest version of the patch.
>
> Yes, I think that's broadly the approach Tom was recommending.

I don't have problem with that approach, but the latest patch has
following problems.
1. We are copying chunks of path creation logic, instead of reusing
corresponding functions.
2. There are many cases where the new function would return no local
path and hence postgres_fdw doesn't push down the join [1]. This will
be performance regression compared to 9.6.

[1] https://www.mail-archive.com/pgsql-hackers@postgresql.org/msg302463.html
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Parallel Index Scans

2017-01-18 Thread Haribabu Kommi

On Wed, Jan 18, 2017 at 6:55 PM, Rahila Syed  wrote:

> >+ /* Check if the scan for current scan keys is finished */
> >+ if (so->arrayKeyCount < btscan->btps_arrayKeyCount)
> >+ *status = false;
>
> >I didn't clearly understand, in which scenario the arrayKeyCount is less
> >than btps_arrayKeyCount?
> Consider following array scan keys
>
> select * from test2 where j=ANY(ARRAY[1000,2000,3000]);
>
> By the time the current worker has finished reading heap tuples
> corresponding
> to array key 1000(arrayKeyCount = 0), some other worker might have
> advanced the scan to the
> array key 2000(btps_arrayKeyCount =1). In this case when the current
> worker fetches next page to scan,
> it must advance its scan keys before scanning the next page of parallel
> scan.
> I hope this helps.
>

Thanks for the details.
One worker incremented arrayKeyCount and btps_arrayKeyCount both. As
btps_arrayKeyCount present in shared memory, so the other worker see the
update
and hits above the condition.


> >+BlockNumber
> >+_bt_parallel_seize(IndexScanDesc scan, bool *status)
>
> >The return value of the above function is validated only in _bt_first
> >function, but in other cases it is not.
> In other cases it is validated in _bt_readnextpage() which is called after
> _bt_parallel_seize().
>
> >From the function description,
> >it is possible to return P_NONE for the workers also with status as
> >true. I feel it is better to handle the P_NONE case internally only
> >so that callers just check for the status. Am i missing anything?
>
> In case of the next block being P_NONE and status true, the code
> calls _bt_parallel_done() to notify other workers followed by
> BTScanPosInvalidate(). Similar check for block = P_NONE also
> happens in existing code. See following in _bt_readnextpage(),
>
>
> if (blkno == P_NONE || !so->currPos.moreRight)
> {
>_bt_parallel_done(scan);
> BTScanPosInvalidate(so->currPos);
> return false;
> }
> So to keep it consistent with the existing code, the check
> is kept outside _bt_parallel_seize().
>

Thanks. Got it.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] PoC: Grouped base relation

2017-01-18 Thread Ashutosh Bapat

On Thu, Jan 19, 2017 at 12:02 AM, Robert Haas  wrote:
> On Tue, Jan 17, 2017 at 11:33 PM, Ashutosh Bapat
>  wrote:
>> I don't think aggcombinefn isn't there because we couldn't write it
>> for array_agg() or string_agg(). I guess, it won't be efficient to
>> have those aggregates combined across parallel workers.
>
> I think there are many cases where it would work fine.  I assume that
> David just didn't make it a priority to write those functions because
> it wasn't important to the queries he wanted to optimize.  But
> somebody can submit a patch for it any time and I bet it will have
> practical use cases.  There might be some performance problems shoving
> large numbers of lengthy values through a shm_mq, but we won't know
> that until somebody tries it.
>
>> Also, the point is naming that kind of function as aggtransmultifn
>> would mean that it's always supposed to multiply, which isn't true for
>> all aggregates.
>
> TransValue * integer = newTransValue is well-defined for any
> aggregate.  It's the result of aggregating that TransValue with itself
> a number of times defined by the integer.  And that might well be
> significantly faster than using aggcombinefn many times.  On the other
> hand, how many queries just sit there are re-aggregate the same
> TransValues over and over again?  I am having trouble wrapping my head
> around that part of this.

Not all aggregates have TransValue * integer = newTransValue
behaviour. Example is array_agg() or string_agg() has "TransValue
concatenated integer time" behaviour. Or an aggregate "multiplying"
values across rows will have TransValue (raised to) integer behaviour.
Labelling all of those as "multi" doesn't look good.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

2017-01-18 Thread Ryan Murphy

> The on-screen output isn't all that helpful for diagnosing what went
> wrong.  You might learn more by looking at the regression.diffs files.
> Remember that errors tend to cascade, so the first one(s) in any
> particular test suite are the most important --- the rest might just
> be fallout.
>
>
Thanks Tom, I'll check out the diffs.

Re: [HACKERS] Patch to implement pg_current_logfile() function

2017-01-18 Thread Michael Paquier

On Thu, Jan 19, 2017 at 6:56 AM, Karl O. Pinc  wrote:
> On Wed, 18 Jan 2017 15:08:09 -0600
> "Karl O. Pinc"  wrote:
>
>> I would like to see index entries for "current_logfiles"
>> so this stuff is findable.
>
> Attached is a v27 of the patch.
>
> I polished some of the sentences in the documentation
> and added index entries.
>
> Also attached are a series of 2 patches to apply on top
> of v27 which make symbols out of hardcoded constants.

+ 
+   current_logfiles
+   and the log_destination configuration parameter
+ 
I have been wondering about this portion a bit, and actually that's pretty nice.

So patch is marked as ready for committer, the version proposed here
not using SRFs per Gilles' input on the matter.
-- 
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] macaddr 64 bit (EUI-64) datatype support

2017-01-18 Thread Haribabu Kommi

On Sat, Jan 14, 2017 at 6:28 PM, Kuntal Ghosh 
wrote:

> On Mon, Jan 9, 2017 at 1:45 PM, Haribabu Kommi 
> wrote:
> > Updated patch is attached.
> >
> I've a few comments about the patch.
>

Thanks for the review.

+ This type can accept both 6 and 8 bytes length MAC addresses.
> A 6 bytes length MAC address is internally converted to 8 bytes. We
> should include this in the docs. Because output is always 8 bytes.
> Otherwise, a user may be surprised with the output.
>

updated accordingly.


> +#define hibits(addr) \
> +  ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)|((addr)
> ->d)))
> +
> +#define lobits(addr) \
> +  ((unsigned long)(((addr)->e<<24)|((addr)->f<<16)|((addr)->g<<8)|((addr)
> ->h)))
> +
> There should be some spacing.
>

corrected.

+ if (!eight_byte_address)
> + {
> + d = 0xFF;
> + e = 0xFE;
> + }
> You already have count variable. Just check (count != 8).
>

Changed.


> + res *= 256 * 256;
> + res *= 256 * 256;
> Bit shift operator can be used for this. For example: (res << 32).
>

Changed by adding a typecast, because res is a double datatype value.


> -DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984  829 t 0 ));
> -DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985  829 t 0 ));
> +DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984  829 t 0 ));
> +DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985  829 t 0 ));
> This is unnecessary I guess.
>

Corrected.


> There was some whitespace error while applying the patch. Also, there
> are some spacing inconsistencies in the comments. I've not tested with
> this patch. I'll let you know once I'm done testing.
>

Corrected.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia


mac_eui64_support_6.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] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

2017-01-18 Thread Tom Lane

Ryan Murphy  writes:
> So I ran "make -i installcheck-world" and it ran to completion.  This is on
> a freshly "git pull"ed postgres source tree.  Certain tests failed, but
> most succeeded.

Those results look pretty broken :-(

> There was no "overall" indication of success or failure at
> the very end, so I'm not sure what this means.

You told make to ignore errors (with "-i"), so it didn't notice that
anything was wrong.

The on-screen output isn't all that helpful for diagnosing what went
wrong.  You might learn more by looking at the regression.diffs files.
Remember that errors tend to cascade, so the first one(s) in any
particular test suite are the most important --- the rest might just
be fallout.

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] smallint out of range EXECUTEing prepared statement

2017-01-18 Thread Andrew Gierth

> "Justin" == Justin Pryzby  writes:

 Justin> Is this expected behavior ?

 Justin> ts=# SELECT * FROM t WHERE site_id=32768 LIMIT 1;
 Justin> (0 rows)

 Justin> ts=# PREPARE x AS SELECT * FROM t WHERE site_id=$1 LIMIT 1;
 Justin> PREPARE
 Justin> ts=# EXECUTE x(32768);
 Justin> ERROR:  smallint out of range

If column "site_id" is of type smallint, then parse analysis will deduce
a type of smallint for $1, which is otherwise of unknown type. So the
prepared statement "x" then has one parameter of type smallint.

Passing 32768 for that parameter therefore fails with the expected error.

 Justin> ts=# PREPARE y AS SELECT * FROM t WHERE site_id::int=$1 LIMIT 1;
 Justin> PREPARE

Now $1 is of type integer, not smallint, because parse analysis sees
(integer = unknown) and deduces the type from that.

(a better way would be WHERE site_id = $1::integer, which would allow
index usage on site_id, unlike your example)

-- 
Andrew (irc:RhodiumToad)


-- 
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] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

2017-01-18 Thread Ryan Murphy

>
> Jim Nasby said I shouldn't necessarily need to build the docs / the whole
> world in order to review patches.  But the Review form needs a `make
> installworld-check`.  Do I need to install the whole world in order to meet
> this requirement?  Happy to do so if required, but in that case, I wonder
> why 'osx' is having so much trouble parsing the SGML into XML?
>

Aha, I was able to run a "make -i install-world", which ignored the SGML
errors and built the rest of the world.  Now trying "make -i
installcheck-world".

Re: [HACKERS] Re: Clarifying "server starting" messaging in pg_ctl start without --wait

2017-01-18 Thread Ryan Murphy

>
>
> Ryan try to run 'make install-world' then 'make -i installcheck-world', -i
> option will ignore the error and proceed. You can check if any other tests
> fails. This is a separate issue, unrelated to this patch. I do not think
> we should stop from changing the status because of this.
>
>
Beena, when I try to run 'make install-world' I get a lot of errors from
the 'osx' executable which I think has to do with building the SGML
documentation:

make -C doc install
make -C src install
make -C sgml install
{ \
  echo ""; \
  echo ""; \
} > version.sgml
'/opt/local/bin/perl' ./mk_feature_tables.pl YES
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-supported.sgml
'/opt/local/bin/perl' ./mk_feature_tables.pl NO
../../../src/backend/catalog/sql_feature_packages.txt
../../../src/backend/catalog/sql_features.txt > features-unsupported.sgml
'/opt/local/bin/perl' ./generate-errcodes-table.pl
../../../src/backend/utils/errcodes.txt > errcodes-table.sgml
/usr/local/bin/osx -D. -x lower -i include-xslt-index postgres.sgml
>postgres.xmltmp
/usr/local/bin/osx:postgres.sgml:3:55:W: cannot generate system identifier
for public text "-//OASIS//DTD DocBook V4.2//EN"
/usr/local/bin/osx:postgres.sgml:12:0:E: reference to entity "BOOK" for
which no system identifier could be generated
/usr/local/bin/osx:postgres.sgml:3:0: entity was defined here
/usr/local/bin/osx:postgres.sgml:12:0:E: DTD did not contain element
declaration for document type name
/usr/local/bin/osx:postgres.sgml:14:9:E: there is no attribute "ID"
/usr/local/bin/osx:postgres.sgml:14:19:E: element "BOOK" undefined
/usr/local/bin/osx:postgres.sgml:15:7:E: element "TITLE" undefined
/usr/local/bin/osx:postgres.sgml:17:10:E: element "BOOKINFO" undefined
/usr/local/bin/osx:postgres.sgml:18:13:E: element "CORPAUTHOR" undefined
/usr/local/bin/osx:postgres.sgml:19:14:E: element "PRODUCTNAME" undefined
/usr/local/bin/osx:postgres.sgml:20:16:E: element "PRODUCTNUMBER" undefined
/usr/local/bin/osx:legal.sgml:3:5:E: element "DATE" undefined
/usr/local/bin/osx:legal.sgml:5:10:E: element "COPYRIGHT" undefined
/usr/local/bin/osx:legal.sgml:6:6:E: element "YEAR" undefined
...
/usr/local/bin/osx:history.sgml:173:13:E: element "LISTITEM" undefined
/usr/local/bin/osx:history.sgml:174:10:E: element "PARA" undefined
/usr/local/bin/osx:history.sgml:175:14:E: element "ACRONYM" undefined
/usr/local/bin/osx:I: maximum number of errors (200) reached; change with
-E option
make[3]: *** [postgres.xml] Error 1
make[2]: *** [install] Error 2
make[1]: *** [install] Error 2
make: *** [install-world-doc-recurse] Error 2


Jim Nasby said I shouldn't necessarily need to build the docs / the whole
world in order to review patches.  But the Review form needs a `make
installworld-check`.  Do I need to install the whole world in order to meet
this requirement?  Happy to do so if required, but in that case, I wonder
why 'osx' is having so much trouble parsing the SGML into XML?

Thanks for the help,
Ryan

Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

2017-01-18 Thread Stephen Frost

* Petr Jelinek (petr.jeli...@2ndquadrant.com) wrote:
> On 10/01/17 17:33, Matheus de Oliveira wrote:
> > 
> > On Mon, Jan 9, 2017 at 10:58 AM, Ashutosh Sharma  > > wrote:
> > 
> > > Also, should I add translations for that error message in other 
> > languages (I
> > > can do that without help of tools for pt_BR) or is that a latter 
> > process in
> > > the releasing?
> > >
> > 
> > I think you should add it but i am not sure when it is done.
> > 
> > 
> > I'll ask one of the guys who work with pt_BR translations (I know him in
> > person).
> 
> Translations are not handled by patch author but by translation project
> so no need.
> 
> > 
> > Attached a rebased version and with the docs update pointed by Ashutosh
> > Sharma.
> > 
> 
> The patch looks good, the only thing I am missing is tab completion
> support for psql.

Awesome, glad to hear it.  This is also on my list of patches that I'm
planning to look at, just so folks know.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

2017-01-18 Thread Petr Jelinek

On 10/01/17 17:33, Matheus de Oliveira wrote:
> 
> On Mon, Jan 9, 2017 at 10:58 AM, Ashutosh Sharma  > wrote:
> 
> > Also, should I add translations for that error message in other 
> languages (I
> > can do that without help of tools for pt_BR) or is that a latter 
> process in
> > the releasing?
> >
> 
> I think you should add it but i am not sure when it is done.
> 
> 
> I'll ask one of the guys who work with pt_BR translations (I know him in
> person).

Translations are not handled by patch author but by translation project
so no need.

> 
> Attached a rebased version and with the docs update pointed by Ashutosh
> Sharma.
> 

The patch looks good, the only thing I am missing is tab completion
support for psql.

-- 
  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] Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

2017-01-18 Thread Tom Lane

Robert Haas  writes:
> So, one of the big reasons I use CASE is to avoid evaluating
> expressions in cases where they might throw an ERROR.  Like, you know:
> CASE WHEN d != 0 THEN n / d ELSE NULL END
> I guess it's not the end of the world if that only works for
> non-set-returning functions, but it's something to think about.

Well, refusing CASE-containing-SRF at all isn't going to make your
life any better in that regard :-(

It's possibly worth noting that this is also true for aggregates and
window functions: wrapping those in a CASE doesn't stop them from being
evaluated, either.  People seem to be generally used to that, although
I think I've seen one or two complaints about it from folks who seemed
unclear on the concept of aggregates.

In the end I think this is mostly about backwards compatibility:
are we sufficiently worried about that that we'd rather throw an
error than have a silent change of behavior?  TBH I'm not sure.
We've certainly got two other silent changes of behavior in this
same patch.  The argument for treating this one differently,
I think, is that it's changing from a less surprising behavior
to a more surprising one whereas the other changes are the reverse,
or at worst neutral.

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

1 2 >

1 - 100 of 111 matches

Mail list logo