Re: [HACKERS] coverage analysis improvements

[Michael Paquier]

On Fri, Sep 22, 2017 at 11:34 PM, Peter Eisentraut
 wrote:
> Apparently, rmgr.c doesn't contain any instrumentable code.  I don't see
> this warning, but it might depend on tool versions and compiler options.

Even on HEAD I am seeing the same problem, this is not outlined just
because the quiet mode of lcov is not used, but the warnings are there
like in this report:
https://www.postgresql.org/message-id/ec92eb95-26de-6da8-9862-ded3ff678c5c%40BlueTreble.com
So please let me discard my concerns about this portion of the patch.

Except for the plperl patch, I don't have more comments to offer about
this patch set. It would be nice to make configure a bit smarter for
lcov and gcov detection by not hard-failing if gcov can be found but
not lcov. It is after all possible to run coverage without lcov, like
on ArchLinux. Still that's a different request than what this patch
set is doing, so this is not a blocker for this patch set.
-- 
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] visual studio 2017 build support

[Haribabu Kommi]

On Mon, Sep 25, 2017 at 10:12 PM, Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
> On 09/25/2017 12:25 AM, Haribabu Kommi wrote:
> >
> > Thanks for pointing it out, I missed to check the Build tools support
> > section.
> > Here I attached the updated patch with the change in documentation to
> > include the 2008 R2 SP1 operating system also.
> >
>
> Thanks, committed and backpatched to 9.6 It would be nice to get
> buildfarm members going supporting  VS2015 and VS2017


Thanks.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] SERIALIZABLE with parallel query

[Haribabu Kommi]

On Mon, Sep 25, 2017 at 6:57 PM, Thomas Munro  wrote:

> On Mon, Sep 25, 2017 at 8:37 PM, Haribabu Kommi
>  wrote:
> > After I tune the GUC to go with sequence scan, still I am not getting the
> > error
> > in the session-2 for update operation like it used to generate an error
> for
> > parallel
> > sequential scan, and also it even takes some many commands until unless
> the
> > S1
> > commits.
>
> Hmm.  Then this requires more explanation because I don't expect a
> difference.  I did some digging and realised that the error detail
> message "Reason code: Canceled on identification as a pivot, during
> write." was reached in a code path that requires
> SxactIsPrepared(writer) and also MySerializableXact == writer, which
> means that the process believes it is committing.  Clearly something
> is wrong.  After some more digging I realised that
> ParallelWorkerMain() calls EndParallelWorkerTransaction() which calls
> CommitTransaction() which calls
> PreCommit_CheckForSerializationFailure().  Since the worker is
> connected to the leader's SERIALIZABLEXACT, that finishes up being
> marked as preparing to commit (not true!), and then the leader get
> confused during that write, causing a serialization failure to be
> raised sooner (though I can't explain why it should be raised then
> anyway, but that's another topic).  Oops.  I think the fix here is
> just not to do that in a worker (the worker's CommitTransaction()
> doesn't really mean what it says).
>
> Here's a version with a change that makes that conditional.  This way
> your test case behaves the same as non-parallel mode.
>

With the latest patch, I didn't find any problems.



> > I will continue my review on the latest patch and share any updates.
>
> Thanks!


The patch looks good, and I don't have any comments for the code.
The test that is going to add by the patch is not generating a true
parallelism scenario, I feel it is better to change the test that can
generate a parallel sequence/index/bitmap scan.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 21:38, Andres Freund wrote:

On 2017-09-25 21:31:11 +0300, Alvaro Hernandez wrote:

- Distribution and testing are non-trivial: many OS/archs combinations.


Yes, it is. Why would we want to increase that burden to this community?


     That's a different story, and one I cannot argue against. If easying
postgresql integration with other tools is not something of a priority or
something the core group cannot add to all the stuff on their shoulders, all
my due respect. PostgreSQL users will do without, someway, somehow. But IMHO
this should be a really high priority, and saying that this would turn
PostgreSQL into an Oracle code base is going too far ;)

Well, we can certainly use more help doing maintenance-y stuff, which
will in turn allow to get more stuff into core in a good state medium+
term... ;)

Less jokingly: I'm doubtful that the "not a priority" take is really
fair - there's a lot of priorities, and they compete for scant
resources. Which means people have to argue convincingly if they want to
add to that burden - just actually asking the question whether it's a
good use of resources doesn't mean it's not. Just that it's not yet
clear.




    OK, let me try to do that. I believe data integration is a 
priority. World is no longer an isolated database where many apps talk 
to it. Nowdays heterogeneous architectures are almost the norm. CDC is 
often the best solution for many of the data integration tasks like data 
warehousing, data consolidation, stream processing, etc. From this 
perspective, it would be key to have a general tool or good starting 
point for CDC or even higher level functionality tools. Think of 
PostgreSQL's Golden Gate.


    Now, developing any software like this faces two significant 
challenges, which turn into deterrents for developing such software:


- If you want to develop your own output plugin, then your market is 
reduced as you have to exclude all the managed solutions (until, and 
only if, you would convince them to include your plugin... highly 
unlikely, very difficult). And probably another % of enterprise 
environments which will hesitate to link your own plugin to the running 
production database. Last but not least, you need to compile and test 
(and testing this is far from easy) on multiple OSes/architectures.


- If you stick to in-core plugins, then you need to support at least 
three different output formats if you want to support 9.4+: 
test_decoding (and pray it works!), pgoutput, and the "new" in-core 
plugin that was proposed at the beginning of this thread, if that would 
see the light.



    Both are strong enough arguments to make building this kind 
software far less interesting. Actually, I don't know of any software 
like this, and this may already be a consequence of what I'm saying. And 
I truly believe PostgreSQL should offer this, as part of its ecosystem 
(not necessarily in core).


    On the other hand, in-core may help encourage building this 
solutions. If there would be an in-core, uniform, flexible, output 
plugin, and ideally backported to 9.4 (I know, I know), included in 
very PostgreSQL... it would really open the doors to many integration 
applications. Output plugins are great for your own use, for controlled 
environments, for specific set of users. But only in-core plugins help 
for general-purpose solutions.



    Álvaro

--

Alvaro Hernandez


---
OnGres



--
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] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 22:08, Jignesh Shah wrote:



On Mon, Sep 25, 2017 at 11:37 AM, Joshua D. Drake 
mailto:j...@commandprompt.com>> wrote:


On 09/25/2017 11:31 AM, Alvaro Hernandez wrote:



Whether or not they are included in a managed environment
is generally based on two things:

1. Safety (why RDS doesn't allow certain C extensions)
2. Community/Popularity (Exactly why RDS has PostGIS)
    A. Demand with a prerequisite of #1


 This is very clear. Now tell me: how many output plugins
do you see included in RDS. And in GCP's PostgreSQL? Azure
Postgres? Heroku?


From RDS:

Logical Replication for PostgreSQL on Amazon RDS

Beginning with PostgreSQL version 9.4, PostgreSQL supports the
streaming of WAL changes using logical replication slots. Amazon
RDS supports logical replication for a PostgreSQL DB instance
version 9.4.9 and higher and 9.5.4 and higher. Using logical
replication, you can set up logical replication slots on your
instance and stream database changes through these slots to a
client like pg_recvlogical. Logical slots are created at the
database level and support replication connections to a single
database.

PostgreSQL logical replication on Amazon RDS is enabled by a new
parameter, a new replication connection type, and a new security
role. The client for the replication can be any client that is
capable of establishing a replication connection to a database on
a PostgreSQL DB instance.

The most common clients for PostgreSQL logical replication are AWS
Database Migration Service or a custom-managed host on an AWS EC2
instance. The logical replication slot knows nothing about the
receiver of the stream; there is no requirement that the target be
a replica database. Note that if you set up a logical replication
slot and do not read from the slot, data can be written to your DB
instance's storage and you can quickly fill up the storage on your
instance.

"""

I don't see why others wouldn't be available either. In fact, I am
not sure why you couldn't use the JSON ones now. (Although I have
not tested it).

JD





Also to add, Amazon RDS for PostgreSQL does supports non-core 
plugins.  Wal2json output plugin for logical decoding is supported for 
versions 9.6.3+ and 9.5.7+  (link 
) 
.




    I think that's awesome. Now... back to my original question: what 
is the *list* of output plugins supported by managed PostgreSQL 
solutions? So far it looks like wal2json for 9.5-9.6 on RDS, and nothing 
else (it may just be not complete, but in the best case this list won't 
be unfortunately long...).



    Álvaro


--

Alvaro Hernandez


---
OnGres

Re: [HACKERS] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 22:13, Magnus Hagander wrote:
On Mon, Sep 25, 2017 at 8:20 PM, Alvaro Hernandez > wrote:




On 25/09/17 20:18, Andres Freund wrote:

On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:

 However, if DMS uses it for what I'd call production
use, I assume it is
actually production quality. I bet they do enough testing,
and don't ship
software to potentially millions of customers if it
doesn't work well. So...
first, I'd consider this a a sign of robustness.

You've been in software for how long? ... ;)  There's quite mixed
experiences with DMS.


    Actually long enough to understand that if someone "big" calls
it production quality, we should not be pickier and assume it is
--whether it is or not. People will accept it as such, and that's
good enough.


Historically the fact that we have been pickier than many of the 
"someone big":s is exactly how we ended up with the codebase and 
relative stability we have today.


Just because someone is big doesn't mean they know what's right. In 
fact, more often than not the opposite turns out to be true.





    Note that I'm not here supporting test_decoding. I'm just saying is 
all what is available in-core for 9.4-9.6, and it seems someone with 
potentially a lot of users tested it and is using it in its own 
solution. Ask me if I would like an in-core, well tested, performant, 
with an easy to parse format, and efficient, for 9.4-9.6? My answer 
would be an immediate 'yes'. But since this is not going to happen, 
test_decoding is good that is good enough, lucky us, because otherwise 
there would not be a good solution on this front.


    Álvaro

--

Alvaro Hernandez


---
OnGres

Re: [HACKERS] moving some partitioning code to executor

[Amit Langote]

On 2017/09/14 16:13, Amit Langote wrote:
> Hi.
> 
> It seems to me that some of the code in partition.c is better placed
> somewhere under the executor directory.  There was even a suggestion
> recently [1] to introduce a execPartition.c to house some code around
> tuple-routing.
> 
> IMO, catalog/partition.c should present an interface for handling
> operations on a *single* partitioned table and avoid pretending to support
> any operations on the whole partition tree.  For example, the
> PartitionDispatch structure embeds some knowledge about the partition tree
> it's part of, which is useful when used for tuple-routing, because of the
> way it works now (lock and form ResultRelInfos of *all* leaf partitions
> before the first input row is processed).
> 
> So, let's move that structure, along with the code that creates and
> manipulates the same, out of partition.c/h and to execPartition.c/h.
> Attached patch attempts to do that.
> 
> While doing the same, I didn't move *all* of get_partition_for_tuple() out
> to execPartition.c, instead modified its signature as shown below:
> 
> -extern int get_partition_for_tuple(PartitionDispatch *pd,
> -TupleTableSlot *slot,
> -EState *estate,
> -PartitionDispatchData **failed_at,
> -TupleTableSlot **failed_slot);
> +extern int get_partition_for_tuple(Relation relation, Datum *values,
> +bool *isnull);
> 
> That way, we keep the core partition bound comparison logic inside
> partition.c and move rest of the stuff to its caller ExecFindPartition(),
> which includes navigating the enveloping PartitionDispatch's.
> 
> Thoughts?
> 
> PS: 0001 of the attached is the patch from [2] which is here to be applied
> on HEAD before applying the main patch (0002) itself

Since that 0001 patch was committed [1], here is the rebased patch.  Will
add this to the November commit-fest.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=77b6b5e9c
From 08b337436b5862b0ee593314864d6ba98f95e6c0 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 8 Sep 2017 19:07:38 +0900
Subject: [PATCH] Move certain partitioning code to the executor

---
 src/backend/catalog/partition.c| 438 +-
 src/backend/commands/copy.c|   1 +
 src/backend/executor/Makefile  |   2 +-
 src/backend/executor/execMain.c| 265 +---
 src/backend/executor/execPartition.c   | 559 +
 src/backend/executor/nodeModifyTable.c |   1 +
 src/include/catalog/partition.h|  48 +--
 src/include/executor/execPartition.h   |  65 
 src/include/executor/executor.h|  14 +-
 9 files changed, 708 insertions(+), 685 deletions(-)
 create mode 100644 src/backend/executor/execPartition.c
 create mode 100644 src/include/executor/execPartition.h

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 1ab6dba7ae..903c8c4def 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -147,8 +147,6 @@ static int32 partition_bound_cmp(PartitionKey key,
 static int partition_bound_bsearch(PartitionKey key,
PartitionBoundInfo boundinfo,
void *probe, bool 
probe_is_bound, bool *is_equal);
-static void get_partition_dispatch_recurse(Relation rel, Relation parent,
-  List **pds, List 
**leaf_part_oids);
 
 /*
  * RelationBuildPartitionDesc
@@ -1193,148 +1191,6 @@ get_partition_qual_relid(Oid relid)
return result;
 }
 
-/*
- * RelationGetPartitionDispatchInfo
- * Returns information necessary to route tuples down a partition 
tree
- *
- * The number of elements in the returned array (that is, the number of
- * PartitionDispatch objects for the partitioned tables in the partition tree)
- * is returned in *num_parted and a list of the OIDs of all the leaf
- * partitions of rel is returned in *leaf_part_oids.
- *
- * All the relations in the partition tree (including 'rel') must have been
- * locked (using at least the AccessShareLock) by the caller.
- */
-PartitionDispatch *
-RelationGetPartitionDispatchInfo(Relation rel,
-int 
*num_parted, List **leaf_part_oids)
-{
-   List   *pdlist = NIL;
-   PartitionDispatchData **pd;
-   ListCell   *lc;
-   int i;
-
-   Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
-
-   *num_parted = 0;
-   *leaf_part_oids = NIL;
-
-   get_partition_dispatch_recurse(rel, NULL, &pdlist, leaf_part_oids);
-   *num_parted = list_length(pdlist);
-   pd = (PartitionDispatchData **) palloc(*num_parted *
-   
   sizeof(PartitionDispatchData

[HACKERS] Runtime Partition Pruning

[Beena Emerson]

I have been working on implementing the runtime partition pruning
which would increase the performance of queries involving partitioned
table to a great extent.

PFA the POC which can be applied over Amit's patch for faster
partition pruning [1] and Dilip's refactor patch [2] on commit
2c74e6c1dcc5002fa8b822e5757f6c95d899fb7a.

[1] 
https://www.postgresql.org/message-id/e02923ea-a117-a6ad-6a3e-ea5e1ba41ece%40lab.ntt.co.jp

[2] 
https://www.postgresql.org/message-id/CAFiTN-tGnQzF_4QtbOHT-3hE%3DOvNaMfbbeRxa4UY0CQyF0G8gQ%40mail.gmail.com

There were a couple of things that need improvement/opinion:
In get_rel_partition_info, we store minop and maxop for each partition
key. For the equality case, which is most common, both would store the
same value. We could make it better by storing equal (bound, bound,
) instead repeating the same values.

get_partitions_for_keys currently returns the list of partitions valid
for the given keys but for a table with many partitions this list
would be very long so maybe for range qual ( key > a & key < b ) we
could only store the min and max partition number and increment
as_whichplan by 1 till we reach max partition number. For
non-continuous partitions, we would still need the list.

Currently, the partitions numbers are recalculated whenever the
ChgParam is set, This can be optimised by skipping this step when only
a non-partition key column has changed; reusing the existing
partitions selected.

Others:
- better handling of multiple key
- allow use of expression in the quals.
- To use min_incl, max_incl properly in get_partitions_for_keys.
- pruning during function calls.


Currently with patch, during NestLoop:
Nested Loop
-> SeqScan tbl1
-> Append
  -> Index Scan p01
  -> Index Scan p02
  -> Index Scan p03

For each tuple from tbl1, only the relevant partition (p01or p02 or
p03) will be scanned.


--- Prepared Statement Behaviour with patch---

Table Descritpion:
   Table "public.tprt"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
+-+---+--+-+-+--+-
 col1   | integer |   |  | | plain   |  |
 col2   | integer |   |  | | plain   |  |
Partition key: RANGE (col1)
Partitions: tprt_1 FOR VALUES FROM (1) TO (50001),
tprt_2 FOR VALUES FROM (50001) TO (11),
tprt_3 FOR VALUES FROM (11) TO (21)

EXPLAIN EXECUTE prstmt_select(15);

QUERY PLAN
--
 Append  (cost=0.00..1736.55 rows=1 width=8)
   ->  Seq Scan on tprt_1  (cost=0.00..849.15 rows=16724 width=8)
 Filter: (col1 < $1)
(3 rows)

EXPLAIN EXECUTE prstmt_select(6);
QUERY PLAN
--
 Append  (cost=0.00..1736.55 rows=2 width=8)
   ->  Seq Scan on tprt_1  (cost=0.00..849.15 rows=16724 width=8)
 Filter: (col1 < $1)
   ->  Seq Scan on tprt_2  (cost=0.00..849.15 rows=16724 width=8)
 Filter: (col1 < $1)
(5 rows)


-- 

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


0001-POC-Implement-runtime-partiton-pruning.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] Setting pd_lower in GIN metapage

[Michael Paquier]

On Mon, Sep 25, 2017 at 3:48 PM, Amit Langote
 wrote:
> Trying to catch up.

Glad to see you back.

> Just to remind, I didn't actually start this thread [1] to address the
> issue that the FPWs of meta pages written to WAL are not getting
> compressed.  An external backup tool relies on pd_lower to give the
> correct starting offset of the hole to compress, provided the page's other
> fields suggest it has the standard page layout.  Since we discovered that
> pd_lower is not set correctly in gin, brin, and spgist meta pages, I
> created patches to do the same.  You (Michael) pointed out that that
> actually helps compress their FPW images in WAL as well, so we began
> considering that.  Also, you pointed out that WAL checking code masks
> pages based on the respective masking functions' assumptions about the
> page's layout properties, which the original patches forgot to consider.
> So, we updated the patches to change the respective masking functions to
> mask meta pages as pages with standard page layout, too.

Thanks for summarizing all that happened.

> But as Tom pointed out [2], WAL compressibility benefit cannot be obtained
> unless we change how the meta page is passed to xlog.c to be written to
> WAL.  So, we found out all the places that write/register the meta page to
> WAL and changed the respective XLogRegisterBuffer calls to include the
> REGBUG_STANDARD flag.  Some of these calls already passed
> REGBUF_WILL_INIT, which would result in no FPW image to be written to the
> WAL and so there would be no question of compressibility.  But, passing
> REGBUF_STANDARD would still help the case where WAL checking is enabled,
> because FPW image *is* written in that case.

Yep.

> So, ISTM, comments that the patches add should all say that setting the
> meta pages' pd_lower to the correct value helps to pass those pages to
> xlog.c as compressible standard layout pages, regardless of whether they
> are actually passed that way.  Even if the patches do take care of the
> latter as well.
>
> Did I miss something?

Not that I think of.

Buffer  metabuffer;
+   Pagemetapage;
SpGistMetaPageData *metadata;

metabuffer = ReadBuffer(index, SPGIST_METAPAGE_BLKNO);
+   metapage = BufferGetPage(metabuffer);
No need to define metapage here and to call BufferGetPage() as long as
the lock on the buffer is not taken.

Except that small thing, the patches do their duty.
-- 
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] Enhancements to passwordcheck

[Bossart, Nathan]

On 9/25/17, 8:31 PM, "Michael Paquier"  wrote:
> Yes, I have developped a couple of years back a fork of passwordcheck
> which had much similar enhancements, so getting something more modular
> in upstream would be really welcome.

Awesome.

> On top of that I think that you should have a parameter that specifies
> a string full of special characters. For example I have been using
> things like !@#$%^&*()_+{}|<>?=. But you likely want to make that
> modular, people have their own set of character characters, and it is
> just something that could be compared with strchr(), still the default
> should be meaningful.

+1

>> passwordcheck.superuser_can_bypass
>
> Not sure that this one has much meaning. Superusers could easily
> unload the module.

True.  I can leave it out for now.

>> passwordcheck.force_new_password
>
> So this is to make sure that the new password is not the same as the
> old one? This would be better with the last N passwords, still this
> would require more facilities. I would discard this one as what you
> are proposing here is not modular enough IMO, and needs a separate
> feature set.

Fair enough.  I'll definitely start with a set of very non-controversial
parameters, as this will likely require rewriting most of the module.

>> I'd like to use this thread to gauge community interest in adding this
>> functionality to this module.  If there is interest, I'll add it to the next
>> commitfest.
>
> I think that's a good idea. Great to see that you are contributing
> back some stuff.

Thanks for the detailed feedback.

On 9/25/17, 8:37 PM, "Euler Taveira"  wrote:
>> passwordcheck.max_expiry_period
>>
> How would you enforce that? If the password expires, you can log in to
> change the password. If you let him/her to get in and change the
> password, you can't obligate the user to do that. You could send a
> message to remember that the password will expire but you can't
> enforce that (unless you make a change in the protocol).

My idea was to tie into the existing password expiration functionality
(VALID UNTIL in CREATE/ALTER ROLE statements).  I don't think this would
involve any changes to how password expiration works.  Instead, it would
just be a simple check to make sure the specified expiration date is not
too far in the future.

>> passwordcheck.force_new_password
>>
> Does it mean a password different from the old one? +1. It could be
> different from the last 3 passwords but we don't store a password
> history.

Yes.  As Michael pointed out, this might be better to do as a separate
effort since we'll almost certainly need to introduce a way to store
password history.

One interesting design challenge will be how to handle pre-hashed
passwords, since the number of checks we can do on those is pretty
limited.  I'm currently thinking of a parameter that can be used to
block, allow, or force pre-hashed passwords.  If we take that route,
perhaps we will also need to ensure that PostgreSQL fails to start when
invalid combinations are specified (e.g. pre-hashed passwords are forced
and min_password_length is nonzero).  Thoughts?

Also, I imagine we'll want to keep the cracklib and "password cannot
contain role name" features around, although perhaps these could become
configurable like the rest of the options.

Nathan


-- 
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] Replication status in logical replication

[Masahiko Sawada]

On Tue, Sep 26, 2017 at 10:36 AM, Vaishnavi Prabakaran
 wrote:
> Hi,
>
> On Wed, Sep 13, 2017 at 9:59 AM, Daniel Gustafsson  wrote:
>>
>>
>> I’m not entirely sure why this was flagged as "Waiting for Author” by the
>> automatic run, the patch applies for me and builds so resetting back to
>> “Needs
>> review”.
>>
>
> This patch applies and build cleanly and I did a testing with one publisher
> and one subscriber, and confirm that the replication state after restarting
> the server now is "streaming" and not "Catchup".
>
> And, I don't find any issues with code and patch to me is ready for
> committer, marked the same in cf entry.
>

Thank you for the reviewing the patch!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] Simplify ACL handling for large objects and removal of superuser() checks

[Michael Paquier]

On Tue, Sep 26, 2017 at 11:42 AM, Vaishnavi Prabakaran
 wrote:
> I moved the cf entry to "ready for committer", and though my vote is for
> keeping the existing API behavior with write implying read, I let the
> committer decide whether the following behavior change is Ok or not.
> "Reading from a large-object descriptor opened with INV_WRITE is NOT
> possible"

Thanks for the review!
-- 
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] Simplify ACL handling for large objects and removal of superuser() checks

[Vaishnavi Prabakaran]

On Tue, Sep 26, 2017 at 11:45 AM, Michael Paquier  wrote:

> On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran
>  wrote:
> > Yes, I did realize on further reading the patch and what led to the
> > confusion is that in the 3rd patch , updated documentation(copied below)
> > still says that reading from a descriptor opened with INV_WRITE is
> possible.
> > I think we need some correction here to reflect the modified code
> behavior.
> >
> > + or other transactions.  Reading from a descriptor opened with
> > + INV_WRITE or INV_READ |
> > + INV_WRITE returns data that reflects all writes of
> > + other committed transactions as well as writes of the current
> > + transaction.
>
> Indeed, you are right. There is an error here. This should read as
> "INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads
> cannot happen.
>
>
Thanks for correcting.

I moved the cf entry to "ready for committer", and though my vote is
for keeping
the existing API behavior with write implying read, I let the committer
decide whether the following behavior change is Ok or not.
"Reading from a large-object descriptor opened with INV_WRITE is NOT
possible"

Thanks & Regards,
Vaishnavi
Fujitsu Australia.

Re: [HACKERS] Built-in plugin for logical decoding output

[Craig Ringer]

On 26 September 2017 at 01:48, Joshua D. Drake  wrote:

> On 09/25/2017 10:43 AM, Andres Freund wrote:
>
>> On 2017-09-25 10:38:52 -0700, Joshua D. Drake wrote:
>>
>>> On 09/25/2017 10:32 AM, Petr Jelinek wrote:
>>>
 On 25/09/17 19:26, Tom Lane wrote:

> Alvaro Hernandez  writes:
>

>>>
 There is already about 3 million output plugins out there so I think we
 did reasonable job there. The fact that vast majority of that are
 various json ones gives reasonable hint that we should have that one in
 core though.

>>>
>>> And I am sure that 2ndQuadrant would be happy to add it to their version
>>> of
>>> Postgres and maintain it themselves.
>>>
>>> https://www.2ndquadrant.com/en/resources/2ndqpostgres/
>>>
>>
>> This doesn't seem like a good way to argue.
>>
>>
> Sorry, that wasn't supposed to be negative. My point was that 2ndQuadrant
> has a distribution of Postgres that they support.


For what it's worth, it's mostly things that didn't make it into core for a
release, things that got knocked back, backports, etc.

I for one don't want to carry a big delta from core.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Built-in plugin for logical decoding output

[Craig Ringer]

On 26 September 2017 at 01:53, Andres Freund  wrote:

> On 2017-09-25 13:50:29 -0400, Tom Lane wrote:
> > Andres Freund  writes:
> > >> On 25/09/17 19:26, Tom Lane wrote:
> > >>> The problem with this type of argument is that it leads directly to
> the
> > >>> conclusion that every feature users want must be in core.
> >
> > > ... I don't think that should mean that there's no possible output
> > > plugin that could/should be integrated into core.
> >
> > Yeah, my point is just that the argument needs to be about why a
> > *particular* plugin is valuable enough to justify adding it to the
> > core developers' maintenance workload.
>
> +1
>
>
Yep, and that goes for plugins like pglogical too.

I agree we need a json plugin, it's pretty clear that's a widespread need.

But I don't buy the whole argument about "hosted postgres" issues. The
hosted solutions suppliers can simply use 3rd party plugins, like some do
PostGIS already. Trying to push things into core is offloading work onto us
to make their lives easier and I don't much care for that.

Personally I'd be more friendly toward Amazon / Google / etc wanting us to
include things for their convenience if they actually usefully contributed
to development and maintenance of Pg.

-- 
 Craig Ringer

Re: [HACKERS] Setting pd_lower in GIN metapage

[Amit Kapila]

On Mon, Sep 25, 2017 at 12:18 PM, Amit Langote
 wrote:
> Hi.
>
> Trying to catch up.
>
> On 2017/09/25 13:43, Michael Paquier wrote:
>> On Sun, Sep 24, 2017 at 2:25 PM, Amit Kapila  wrote:
>>> Added and updated the comments for both btree and hash index patches.
>>
>> I don't have real complaints about this patch, this looks fine to me.
>>
>> +* Currently, the advantage of setting pd_lower is in limited cases like
>> +* during wal_consistency_checking or while logging for unlogged relation
>> +* as for all other purposes, we initialize the metapage.  Note, it also
>> +* helps in page masking by allowing to mask unused space.
>> I would have reworked this comment a bit, say like that:
>> Setting pd_lower is useful for two cases which make use of WAL
>> compressibility even if the meta page is initialized at replay:
>> - Logging of init forks for unlogged relations.
>> - wal_consistency_checking logs extra full-page writes, and this
>> allows masking of the unused space of the page.
>>
>> Now I often get complains that I suck at this exercise ;)
>
> So, I think I understand the discussion so far and the arguments about
> what we should write to explain why we're setting pd_lower to the correct
> value.
>
> Just to remind, I didn't actually start this thread [1] to address the
> issue that the FPWs of meta pages written to WAL are not getting
> compressed.  An external backup tool relies on pd_lower to give the
> correct starting offset of the hole to compress, provided the page's other
> fields suggest it has the standard page layout.  Since we discovered that
> pd_lower is not set correctly in gin, brin, and spgist meta pages, I
> created patches to do the same.  You (Michael) pointed out that that
> actually helps compress their FPW images in WAL as well, so we began
> considering that.  Also, you pointed out that WAL checking code masks
> pages based on the respective masking functions' assumptions about the
> page's layout properties, which the original patches forgot to consider.
> So, we updated the patches to change the respective masking functions to
> mask meta pages as pages with standard page layout, too.
>
> But as Tom pointed out [2], WAL compressibility benefit cannot be obtained
> unless we change how the meta page is passed to xlog.c to be written to
> WAL.  So, we found out all the places that write/register the meta page to
> WAL and changed the respective XLogRegisterBuffer calls to include the
> REGBUG_STANDARD flag.  Some of these calls already passed
> REGBUF_WILL_INIT, which would result in no FPW image to be written to the
> WAL and so there would be no question of compressibility.  But, passing
> REGBUF_STANDARD would still help the case where WAL checking is enabled,
> because FPW image *is* written in that case.
>
> So, ISTM, comments that the patches add should all say that setting the
> meta pages' pd_lower to the correct value helps to pass those pages to
> xlog.c as compressible standard layout pages, regardless of whether they
> are actually passed that way.  Even if the patches do take care of the
> latter as well.
>
> Did I miss something?
>
> Looking at Amit K's updated patches for btree and hash, it seems that he
> updated the comment to say that setting pd_lower to the correct value is
> *essential*, because those pages are passed as REGBUF_STANDARD pages and
> hence will be compressed.  That seems to contradict what I wrote above,
> but I think that's not wrong, too.
>

I think you are missing that there are many cases where we use only
REGBUF_STANDARD for meta-pages (cf. hash index).  For btree, where the
advantage is in fewer cases, I have explicitly stated those cases.

Do you still have confusion?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: 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] Shaky coding for vacuuming partitioned relations

[Michael Paquier]

On Tue, Sep 26, 2017 at 10:55 AM, Amit Langote
 wrote:
> On 2017/09/26 9:51, Michael Paquier wrote:
>> On Tue, Sep 26, 2017 at 8:48 AM, Michael Paquier
>>  wrote:
>>> On Mon, Sep 25, 2017 at 11:32 PM, Tom Lane  wrote:
 Yeah, I'd noticed that while reviewing the vacuum-multiple-tables patch.
 My thought about fixing it was to pass a null RangeVar when handling a
 table we'd identified through inheritance or pg_class-scanning, to
 indicate that this wasn't a table named in the original command.  This
 only works conveniently if you decide that it's appropriate to silently
 ignore relation_open failure on such table OIDs, but I think it is.

 Not sure about whether we ought to try to fix that in v10.  It's a
 mostly-cosmetic problem in what ought to be an infrequent corner case,
 so it might not be worth taking risks for post-RC1.  OTOH, I would
 not be surprised to get bug reports about it down the road.
>>>
>>> Something like that looks like a good compromise for v10. I would
>>> rather see a more complete fix with each relation name reported
>>> correctly on HEAD though. The information provided would be useful for
>>> users when using autovacuum when skipping a relation because no lock
>>> could be taken on it.
>>
>> Actually, perhaps this should be tracked as an open item? A simple fix
>> is leading to the path that no information is better than misleading
>> information in this case.
>
> +1.

Let's track it then and spawn a separate thread with a patch. Do you
want to work on it or should I? The solution proposed by Tom seems
like the correct answer. I am adding an item for now, we could always
link it to a thread later on.

Let's also track the problem that has been reported on this thread.
-- 
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] Shaky coding for vacuuming partitioned relations

[Michael Paquier]

On Tue, Sep 26, 2017 at 10:54 AM, Amit Langote
 wrote:
> I think that's right, although, I don't see any new RangeVar created under
> vacuum() at the moment.  Maybe, you're referring to the Nathan's patch
> that perhaps does that.

Yes, you can check what it does here (last version):
766556dd-aa3c-42f7-acf4-5dc97f41f...@amazon.com
-- 
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: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

[Michael Paquier]

On Mon, Sep 25, 2017 at 11:22 PM, Peter Eisentraut
 wrote:
> Here is a review of the meat of the code, leaving aside the discussion
> of the libpq connection parameters.
>
> Overall, the structure of the code makes sense and it fits in well with
> the existing SCRAM code.

Thanks for the review, Peter.

> I think the channel-binding negotiation on the client side is wrong.
> The logic in the patch is
>
> +#ifdef USE_SSL
> +   if (state->ssl_in_use)
> +   appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
> +   else
> +   appendPQExpBuffer(&buf, "y");
> +#else
> +   appendPQExpBuffer(&buf, "n");
> +#endif
>
> But if SSL is compiled in but not used, the client does not in fact
> support channel binding (for that connection), so it should send "n".

For others, details about this flag are here:
   gs2-cbind-flag  = ("p=" cb-name) / "n" / "y"
 ;; "n" -> client doesn't support channel binding.
 ;; "y" -> client does support channel binding
 ;;but thinks the server does not.
 ;; "p" -> client requires channel binding.
 ;; The selected channel binding follows "p=".

And channel binding description is here:
https://tools.ietf.org/html/rfc5802#section-6

> The "y" flag should be sent if ssl_in_use but the client did not see the
> server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
> this patch.

Okay. I think I get your point here. I agree that the client is
deficient here. This needs some more work.

> You have the server reject a client that does not support channel
> binding ("n") on all SSL connections.  I don't think this is correct.
> It is up to the client to use channel binding or not, even on SSL
> connections.

It seems that I got confused with the meaning of "y" mixed with
ssl_in_use. The server should reject "y" instead only if SSL is in
use.

> We should update pg_hba.conf to allow a method specification of
> "scram-sha256-plus", i.e., only advertise the channel binding variant to
> the client.  Then you could make policy decisions like rejecting clients
> that do not use channel binding on SSL connections.  This could be a
> separate patch later.

OK, I agree that there could be some value in that. This complicates a
bit hba rule checks, but nothing really complicated either.

> The error message in the "p" case if SSL is not in use is a bit
> confusing: "but the server does not need it".  I think this could be
> left at the old message "but it is not supported".  This ties into my
> interpretation from above that whether channel binding is "supported"
> depends on whether SSL is in use for a particular connection.

Check.

> Some small code things:
> - prefer to use size_t over int for length (tls_finish_len etc.)
> - tls_finish should be tls_finished
> - typos: certificate_bash -> certificate_hash

Yes, thanks for spotting those.

> In the patch for tls-server-end-point, I think the selection of the hash
> function deviates slightly from the RFC.  The RFC only says to
> substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
> for example.  There is also the problem that the code as written will
> turn any unrecognized hash method into SHA-256.  If think the code
> should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
> for the rest.  (I don't know anything about the details of OpenSSL APIs,
> but that function sounded right to me.)

Relevant bits from the RFC: https://tools.ietf.org/html/rfc5929#section-4.1
   o  if the certificate's signatureAlgorithm uses a single hash
  function, and that hash function is either MD5 [RFC1321] or SHA-1
  [RFC3174], then use SHA-256 [FIPS-180-3];
   o  if the certificate's signatureAlgorithm uses no hash functions or
  uses multiple hash functions, then this channel binding type's
  channel bindings are undefined at this time (updates to is channel
  binding type may occur to address this issue if it ever arises).

OK. Hm. I think that we need as well to check for the case where
EVP_get_digestbynid() returns NULL then and issue an ERROR on it. That
seems to be the second point outlined by the RFC I am quoting here.

This patch got the feedback I was looking for, and this requires some
rework anyway. So I am marking the patch as returned with feedback for
now. This won't make it in time for this CF.

Thanks Peter for looking at it and pointing out some deficiencies.
-- 
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] Shaky coding for vacuuming partitioned relations

[Amit Langote]

On 2017/09/26 9:51, Michael Paquier wrote:
> On Tue, Sep 26, 2017 at 8:48 AM, Michael Paquier
>  wrote:
>> On Mon, Sep 25, 2017 at 11:32 PM, Tom Lane  wrote:
>>> Yeah, I'd noticed that while reviewing the vacuum-multiple-tables patch.
>>> My thought about fixing it was to pass a null RangeVar when handling a
>>> table we'd identified through inheritance or pg_class-scanning, to
>>> indicate that this wasn't a table named in the original command.  This
>>> only works conveniently if you decide that it's appropriate to silently
>>> ignore relation_open failure on such table OIDs, but I think it is.
>>>
>>> Not sure about whether we ought to try to fix that in v10.  It's a
>>> mostly-cosmetic problem in what ought to be an infrequent corner case,
>>> so it might not be worth taking risks for post-RC1.  OTOH, I would
>>> not be surprised to get bug reports about it down the road.
>>
>> Something like that looks like a good compromise for v10. I would
>> rather see a more complete fix with each relation name reported
>> correctly on HEAD though. The information provided would be useful for
>> users when using autovacuum when skipping a relation because no lock
>> could be taken on it.
> 
> Actually, perhaps this should be tracked as an open item? A simple fix
> is leading to the path that no information is better than misleading
> information in this case.

+1.

Thanks,
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] Shaky coding for vacuuming partitioned relations

[Amit Langote]

On 2017/09/25 18:37, Michael Paquier wrote:
> On Mon, Sep 25, 2017 at 4:42 PM, Amit Langote
>  wrote:
>> On 2017/09/25 12:10, Michael Paquier wrote:
>> Hmm, I'm not sure if we need to lock the partitions, too.  Locks taken by
>> find_all_inheritors() will be gone once the already-running transaction is
>> ended by the caller (vacuum()).  get_rel_oids() should just lock the table
>> mentioned in the command (the parent table), so that find_all_inheritors()
>> returns the correct partition list, as Tom perhaps alluded to when he said
>> "it would also make the find_all_inheritors call a lot more meaningful.",
>> but...
> 
> It is important to hold the lock for child tables until the end of the
> transaction actually to fill in correctly the RangeVar associated to
> each partition when scanning them.

I think that's right, although, I don't see any new RangeVar created under
vacuum() at the moment.  Maybe, you're referring to the Nathan's patch
that perhaps does that.

>> When vacuum() iterates that list and calls vacuum_rel() for each relation
>> in the list, it might be the case that a relation in that list is no
>> longer a partition of the parent.  So, one might say that it would get
>> vacuumed unnecessarily.  Perhaps that's fine?
> 
> I am personally fine with that. Any checks would prove to complicate
> the code for not much additional value.

Tend to agree here.

>>> I am not
>>> saying that this needs to be fixed in REL_10_STABLE at this stage
>>> though as this would require some refactoring similar to what the
>>> patch adding support for VACUUM with multiple relations does.
>>
>> I think it would be better to fix that independently somehow.  VACUUM on
>> partitioned tables is handled by calling vacuum_rel() on individual
>> partitions.  It would be nice if vacuum_rel() didn't have to refer to the
>> RangeVar.  Could we not use get_rel_name(relid) in place of
>> relation->relname?  I haven't seen the other patch yet though, so maybe
>> I'm missing something.
> 
> The RangeVar is used for error reporting, and once you reach
> vacuum_rel, the relation and its OID may be gone. So you need to save
> the information of the RangeVar beforehand when scanning the
> relations. That's one reason behind having the VacuumRelation stuff
> discussed on the thread for multiple table VACUUM, this way you store
> for each item to be vacuumed:
> - A RangeVar.
> - A list of columns.
> - An OID saved by vacuum deduced from the RangeVar.
> That's quite some refactoring, so my opinion is that this ship has
> sailed already for REL_10_STABLE, and that we had better live with
> that in 10.

Yeah, your simple patch seems enough for v10.

Thanks,
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] Simplify ACL handling for large objects and removal of superuser() checks

[Michael Paquier]

On Tue, Sep 26, 2017 at 9:04 AM, Vaishnavi Prabakaran
 wrote:
> Yes, I did realize on further reading the patch and what led to the
> confusion is that in the 3rd patch , updated documentation(copied below)
> still says that reading from a descriptor opened with INV_WRITE is possible.
> I think we need some correction here to reflect the modified code behavior.
>
> + or other transactions.  Reading from a descriptor opened with
> + INV_WRITE or INV_READ |
> + INV_WRITE returns data that reflects all writes of
> + other committed transactions as well as writes of the current
> + transaction.

Indeed, you are right. There is an error here. This should read as
"INV_READ | INV_WRITE" only. Using "INV_WRITE" implies that reads
cannot happen.
-- 
Michael


0001-Remove-ALLOW_DANGEROUS_LO_FUNCTIONS-for-LO-related-s.patch
Description: Binary data


0002-Replace-superuser-checks-of-large-object-import-expo.patch
Description: Binary data


0003-Move-ACL-checks-for-large-objects-when-opening-them.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] Replication status in logical replication

[Vaishnavi Prabakaran]

Hi,

On Wed, Sep 13, 2017 at 9:59 AM, Daniel Gustafsson  wrote:

>
> I’m not entirely sure why this was flagged as "Waiting for Author” by the
> automatic run, the patch applies for me and builds so resetting back to
> “Needs
> review”.
>
>
This patch applies and build cleanly and I did a testing with one publisher
and one subscriber, and confirm that the replication state after restarting
the server now is "streaming" and not "Catchup".

And, I don't find any issues with code and patch to me is ready for
committer, marked the same in cf entry.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

Re: [HACKERS] Enhancements to passwordcheck

[Euler Taveira]

2017-09-25 15:04 GMT-03:00 Bossart, Nathan :
> Currently, the passwordcheck module provides a few basic checks to strengthen
> passwords.  However, any configuration must be ready at compile time, and many
> common password requirements cannot be enforced without creating a custom
> version of this module.  I think there are a number of useful parameters that
> could be added to enable common password restrictions, including the following
> list, which is based on some asks from our customers:
>
> passwordcheck.min_password_length
> passwordcheck.min_uppercase_letters
> passwordcheck.min_lowercase_letters
> passwordcheck.min_numbers
> passwordcheck.min_special_chars
>
+1.

> passwordcheck.superuser_can_bypass
>
It is not clear if it will bypass the checks for (i) a new superuser
or (ii) a superuser creating a new role. I wouldn't recommend using
such option even tough it is a common practice.

> passwordcheck.max_expiry_period
>
How would you enforce that? If the password expires, you can log in to
change the password. If you let him/her to get in and change the
password, you can't obligate the user to do that. You could send a
message to remember that the password will expire but you can't
enforce that (unless you make a change in the protocol).

> passwordcheck.force_new_password
>
Does it mean a password different from the old one? +1. It could be
different from the last 3 passwords but we don't store a password
history.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


-- 
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] Enhancements to passwordcheck

[Michael Paquier]

On Tue, Sep 26, 2017 at 3:04 AM, Bossart, Nathan  wrote:
> Currently, the passwordcheck module provides a few basic checks to strengthen
> passwords.  However, any configuration must be ready at compile time, and many
> common password requirements cannot be enforced without creating a custom
> version of this module.

Yes, I have developped a couple of years back a fork of passwordcheck
which had much similar enhancements, so getting something more modular
in upstream would be really welcome.

> I think there are a number of useful parameters that
> could be added to enable common password restrictions, including the following
> list, which is based on some asks from our customers:
>
> passwordcheck.min_password_length

Okay. I have that as well.

> passwordcheck.min_uppercase_letters
> passwordcheck.min_lowercase_letters
> passwordcheck.min_numbers

Those map with isdigit(), isupper() and islower(). Exactly what I have.

> passwordcheck.min_special_chars

On top of that I think that you should have a parameter that specifies
a string full of special characters. For example I have been using
things like !@#$%^&*()_+{}|<>?=. But you likely want to make that
modular, people have their own set of character characters, and it is
just something that could be compared with strchr(), still the default
should be meaningful.

> passwordcheck.superuser_can_bypass

Not sure that this one has much meaning. Superusers could easily
unload the module.

> passwordcheck.max_expiry_period

Okay. valid_until is passed down to the password hook.

> passwordcheck.force_new_password

So this is to make sure that the new password is not the same as the
old one? This would be better with the last N passwords, still this
would require more facilities. I would discard this one as what you
are proposing here is not modular enough IMO, and needs a separate
feature set.

> I'd like to use this thread to gauge community interest in adding this
> functionality to this module.  If there is interest, I'll add it to the next
> commitfest.

I think that's a good idea. Great to see that you are contributing
back some stuff.
-- 
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] Shaky coding for vacuuming partitioned relations

[Michael Paquier]

On Tue, Sep 26, 2017 at 8:48 AM, Michael Paquier
 wrote:
> On Mon, Sep 25, 2017 at 11:32 PM, Tom Lane  wrote:
>> Yeah, I'd noticed that while reviewing the vacuum-multiple-tables patch.
>> My thought about fixing it was to pass a null RangeVar when handling a
>> table we'd identified through inheritance or pg_class-scanning, to
>> indicate that this wasn't a table named in the original command.  This
>> only works conveniently if you decide that it's appropriate to silently
>> ignore relation_open failure on such table OIDs, but I think it is.
>>
>> Not sure about whether we ought to try to fix that in v10.  It's a
>> mostly-cosmetic problem in what ought to be an infrequent corner case,
>> so it might not be worth taking risks for post-RC1.  OTOH, I would
>> not be surprised to get bug reports about it down the road.
>
> Something like that looks like a good compromise for v10. I would
> rather see a more complete fix with each relation name reported
> correctly on HEAD though. The information provided would be useful for
> users when using autovacuum when skipping a relation because no lock
> could be taken on it.

Actually, perhaps this should be tracked as an open item? A simple fix
is leading to the path that no information is better than misleading
information in this case.
-- 
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] Simplify ACL handling for large objects and removal of superuser() checks

[Vaishnavi Prabakaran]

Hi,

On Tue, Sep 19, 2017 at 5:12 PM, Michael Paquier 
wrote:

>
> >>@@ -163,22 +150,16 @@ lo_read(int fd, char *buf, int len)
> >> 
> >> + if ((lobj->flags & IFS_RDLOCK) == 0)
> >>+ ereport(ERROR,
> >>+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> >>+ errmsg("large object descriptor %d was not opened for reading",
> >>+ fd)));
> >
> > Do we ever reach this error? Because per my understanding
>
> This error can be reached, and it is part of the regression tests. One
> query which passed previously is now failing:
> +SELECT loread(lo_open(1001, x'2'::int), 32);   -- fail, wrong mode
> +ERROR:  large object descriptor 0 was not opened for reading



Yes, I did realize on further reading the patch and what led to the
confusion is that in the 3rd patch , updated documentation(copied below)
still says that reading from a descriptor opened with INV_WRITE is
possible. I think we need some correction here to reflect the modified code
behavior.

+ or other transactions.  Reading from a descriptor opened with
+ INV_WRITE or INV_READ |
+ INV_WRITE returns data that reflects all writes of
+ other committed transactions as well as writes of the current
+ transaction.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

Re: [HACKERS] Shaky coding for vacuuming partitioned relations

[Bossart, Nathan]

On 9/25/17, 6:51 PM, "Michael Paquier"  wrote:
>> +* Take a lock here for the relation lookup. If ANALYZE or 
>> VACUUM spawn
>> +* multiple transactions, the lock taken here will be gone 
>> once the
>> +* current transaction running commits, which could cause 
>> the relation
>> +* to be gone, or the RangeVar might not refer to the OID 
>> looked up here.
>>
>> I think this could be slightly misleading.  Perhaps it would be more
>> accurate to say that the lock will be gone any time vacuum() creates a new
>> transaction (either in vacuum_rel() or when use_own_xacts is true).
>
> The comment of the proposed patch matches as much as possible what is
> currently on HEAD, so I would still go with something close to that.

Sure.  This is just a minor point, and I could see the argument that your
phrasing is more concise, anyway.

Nathan


-- 
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] Shaky coding for vacuuming partitioned relations

[Michael Paquier]

On Tue, Sep 26, 2017 at 1:47 AM, Bossart, Nathan  wrote:
> On 9/24/17, 10:12 PM, "Michael Paquier"  wrote:
>> Attached is a proposal of patch.
>
> The patch seems reasonable to me, and I haven't encountered any issues in
> my tests, even after applying the vacuum-multiple-relations patch on top
> of it.

Thanks for the review, Nathan!

> +* Take a lock here for the relation lookup. If ANALYZE or 
> VACUUM spawn
> +* multiple transactions, the lock taken here will be gone 
> once the
> +* current transaction running commits, which could cause the 
> relation
> +* to be gone, or the RangeVar might not refer to the OID 
> looked up here.
>
> I think this could be slightly misleading.  Perhaps it would be more
> accurate to say that the lock will be gone any time vacuum() creates a new
> transaction (either in vacuum_rel() or when use_own_xacts is true).

The comment of the proposed patch matches as much as possible what is
currently on HEAD, so I would still go with something close to 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] Shaky coding for vacuuming partitioned relations

[Michael Paquier]

On Mon, Sep 25, 2017 at 11:32 PM, Tom Lane  wrote:
> Yeah, I'd noticed that while reviewing the vacuum-multiple-tables patch.
> My thought about fixing it was to pass a null RangeVar when handling a
> table we'd identified through inheritance or pg_class-scanning, to
> indicate that this wasn't a table named in the original command.  This
> only works conveniently if you decide that it's appropriate to silently
> ignore relation_open failure on such table OIDs, but I think it is.
>
> Not sure about whether we ought to try to fix that in v10.  It's a
> mostly-cosmetic problem in what ought to be an infrequent corner case,
> so it might not be worth taking risks for post-RC1.  OTOH, I would
> not be surprised to get bug reports about it down the road.

Something like that looks like a good compromise for v10. I would
rather see a more complete fix with each relation name reported
correctly on HEAD though. The information provided would be useful for
users when using autovacuum when skipping a relation because no lock
could be taken on it.
-- 
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] COMMIT TRIGGERs, take n, implemented with CONSTRAINT TRIGGERS

[Nico Williams]

I guess this got lost over the weekend and subsequent week (I was on
vacation).

On Fri, Sep 15, 2017 at 04:03:35PM -0500, Nico Williams wrote:
> On Fri, Sep 15, 2017 at 04:07:33PM -0400, Tom Lane wrote:
> > Nico Williams  writes:
> > > On Fri, Sep 15, 2017 at 02:19:29PM -0500, Nico Williams wrote:
> > >> On Fri, Sep 15, 2017 at 11:26:08AM -0700, Andres Freund wrote:
> > >>> I think you should also explain why that's a desirable set of
> > >>> semantics.
> > 
> > > Now, why is this desirable: atomicity.
> > 
> > >The client/user can no longer add statements to the transaction,
> > >therefore now (commit-time) is a good time to run a procedure that
> > >can *atomically* examine the totatility of the changes made by the
> > >user.
> > 
> > I do not really understand this claim.  The argument for a commit trigger
> > seems to be exactly "I want to be the last thing that happens in the
> > transaction".  But as soon as you have two of them, one of them is not
> 
> The user defining the triggers is a very different thing from the user
> sending the various statements up to and including the COMMIT.  They
> need not be the same.
> 
> The DBA/developers/ops own the triggers.  The client user may not have
> any trigger creation privilege.
> 
> Being in control of the [pre-]commit triggers, I can control the order
> in which they run (by-name, lexical order).
> 
> The important thing is that all of the [pre-]commit triggers will run
> after the last statement in the TX send by the client.
> 
> Surely there's nothing strange abolut this -- it's already the case for
> [DEFERRED] CONSTRAINT TRIGGERs!

Not to belabor the point, though I'm being redundant: the important
thing is that we have something we can run after the last statement from
the _client_.  Trigger order is already what it is, and that's fine
because the dev/dba controls that, but does not control what the
_client_ sends.

I'm using PostgreSQL for an all-SQL application with:

 - [WISH] access via the PostgreSQL protocol (unconstrained)

 - [CURRENT] access via HTTP via PostgREST (fairly constrained)

This means we don't have full control over the client.  Even if we did,
the client authenticates as users, and we can't ensure that the users
aren't connecting separately and sending arbitrary commands.

Incidentally, there's also a need for a connect-time or BEGIN-time
trigger to simplify other trigger functions.  I often write trigger
functions that do CREATE TEMP TABLE IF NOT EXISTS -- a handy technique.
But it'd be nice if we could have temp tables (and indexes, and views,
and...) automatically created for each session or transaction.  If
nothing else, it would allow statement/row triggers to do less work,
thus run marginally faster.

Nico
-- 


-- 
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] Row Level Security Documentation

[Stephen Frost]

Dean,

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 5 August 2017 at 10:03, Fabien COELHO  wrote:
> > Patch applies cleanly, make html ok, new table looks good to me.
> 
> So I started looking at this patch, but before even considering the
> new table proposed, I think there are multiple issues that need to be
> resolved with the current docs:
> 
> Firstly, there are 4 separate places in the current CREATE POLICY docs
> that say that multiple policies are combined using OR, which is of
> course no longer correct, since PG10 added RESTRICTIVE policy support.

Ah, indeed, you're right there, those should have been updated to
indicate that it was the default or perhaps clarify the difference, but
I agree that doing so all over the place isn't ideal.

> In fact, it wasn't even strictly correct in 9.5/9.6, because if
> multiple different types of policy apply to a single command (e.g.
> SELECT and UPDATE policies, being applied to an UPDATE command), then
> they are combined using AND. Rather than trying to make this correct
> in 4 different places, I think there should be a single new section of
> the docs that explains how multiple policies are combined. This will
> also reduce the number of places where the pre- and post-v10 docs
> differ, making them easier to maintain.

Agreed, that makes a lot of sense to me.

> The proposed patch distinguishes between UPDATE/DELETE commands that
> have WHERE or RETURNING clauses, and those that don't, claiming that
> the former require SELECT permissions and the latter don't. That's
> based on a couple of statements from the current docs, but it's not
> entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true
> RETURNING now()" does not require SELECT permissions despite having
> both WHERE and RETURNING clauses (OK, I admit that's a rather
> contrived example, but still...), and conversely (a more realistic
> example) "UPDATE foo SET a=b+c" does require SELECT permissions
> despite not having WHERE or RETURNING clauses. I think we need to try
> to be more precise about when SELECT permissions are required.

Agreed.

> In the notes section, there is a note about there needing to be at
> least one permissive policy for restrictive policies to take effect.
> That's well worth pointing out, however, I fear that this note is
> buried so far down the page (amongst some other very wordy notes) that
> readers will miss it. I suggest moving it to the parameters section,
> where permissive and restrictive policies are first introduced,
> because it's a pretty crucial fact to be aware of when using these new
> types of policy.

Ok.

> Attached is a proposed patch to address these issues, along with a few
> other minor tidy-ups.

I've taken a look through this and generally agree with it.  I'll note
that the bits inside  ...  tags should be
consistently capitalized (you had one case of 'All' that I noticed) and
I wonder if it'd be better to have the "simple" description *first*
instead of later on, eg, where you have "Thus the overall result is
that" we might want to try and reword things to decribe it as "Overall,
it works thusly, ..., and the specifics are ...".

That's a relatively minor point, however, and I do feel that this patch
is a definite improvement.  Were you thinking of just applying this for
v10, or back-patching all or part of it..?  For my 2c, at least, I'm
pretty open to clarifying things in the back-branches (and we have
technically had restrictive policies for a while, they just required
using an extension, so even those pieces are relevant for older
versions, but might need additional caveats...).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

[Peter Geoghegan]

On Mon, Sep 25, 2017 at 12:52 PM, Peter Geoghegan  wrote:
> That must have been the real reason why you canonicalized
> pg_collation.collname (I doubt it had anything to do with how keyword
> variants used to be created during initdb, as you suggested). As Tom
> pointed out recently, we've actually always canonicalized collation
> name for libc.

On further examination, none of this really matters, because you
simply cannot store ICU locale names like "en_US" within pg_collation;
it's impossible to do that without breaking many things that have
worked for a long time. initdb already canonicalizes the available
libc collations to produce collations whose names have exactly the
same "en_US" format. There will typically be both "en_US" and
"en_US.utf8" entries within pg_collation with Glibc on Linux, for example
(the former is created a convenient alias for the latter when the
database encoding is UTF-8).

-- 
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Thomas Munro]

On Tue, Sep 26, 2017 at 10:12 AM, Tom Lane  wrote:
> Thomas Munro  writes:
>> I think the problem here is that posix_fallocate() doesn't set errno.
>
> Huh.  So the fact that it worked for me is likely because glibc's
> emulation *does* allow errno to get set.
>
>> Will write a patch.
>
> Thanks, I'm out of time for today.

See attached, which also removes the ENOSYS stuff which I believe to
be now useless.  Does this make sense?  Survives make check-world and
my simple test procedure on a
3.10.0-327.36.1.el7.x86_64 system.

postgres=# select test_dsm(1);
ERROR:  could not resize shared memory segment
"/PostgreSQL.2043796572" to 1 bytes: No space left on
device
postgres=# select test_dsm(1000);
 test_dsm
--

(1 row)

-- 
Thomas Munro
http://www.enterprisedb.com


0001-Fix-error-handling-for-posix_fallocate.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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Tom Lane]

Thomas Munro  writes:
> I think the problem here is that posix_fallocate() doesn't set errno.

Huh.  So the fact that it worked for me is likely because glibc's
emulation *does* allow errno to get set.

> Will write a patch.

Thanks, I'm out of time for today.

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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Thomas Munro]

On Tue, Sep 26, 2017 at 9:57 AM, Thomas Munro
 wrote:
>> On Tue, Sep 26, 2017 at 9:13 AM, Tom Lane  wrote:
>>> Pushed with that change; we'll soon see what the buildfarm thinks.
>
> Hmm.  One failure in the test modules:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2017-09-25%2020%3A45%3A02
>
> 2017-09-25 13:51:00.753 PDT [9107:6] LOG:  worker process: test_shm_mq
> (PID 9362) exited with exit code 1
> 2017-09-25 13:51:00.753 PDT [9360:7] ERROR:  could not resize shared
> memory segment "/PostgreSQL.1896677221" to 65824 bytes: Success
>
> hostname = centos7x.joeconway.com
> uname -r = 3.10.0-229.11.1.el7.x86_64

I think the problem here is that posix_fallocate() doesn't set errno.
It returns an errno-like value.  This is a difference from
fallocate().  Will write a patch.

-- 
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Thomas Munro]

> On Tue, Sep 26, 2017 at 9:13 AM, Tom Lane  wrote:
>> Pushed with that change; we'll soon see what the buildfarm thinks.

Hmm.  One failure in the test modules:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2017-09-25%2020%3A45%3A02

2017-09-25 13:51:00.753 PDT [9107:6] LOG:  worker process: test_shm_mq
(PID 9362) exited with exit code 1
2017-09-25 13:51:00.753 PDT [9360:7] ERROR:  could not resize shared
memory segment "/PostgreSQL.1896677221" to 65824 bytes: Success

hostname = centos7x.joeconway.com
uname -r = 3.10.0-229.11.1.el7.x86_64

-- 
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Thomas Munro]

On Tue, Sep 26, 2017 at 9:13 AM, Tom Lane  wrote:
> I wrote:
>> Rather than dig into the guts of glibc to find that out, though, I think
>> we should just s/fallocate/posix_fallocate/g on this patch.  The argument
>> for using the former seemed pretty thin to begin with.
>
> Pushed with that change; we'll soon see what the buildfarm thinks.

Thanks.

> I suspect that the provisions for EINTR and ENOSYS errors may now be
> dead code, since the Linux man page for posix_fallocate mentions
> neither.  They're not much code though, and POSIX itself *does*
> list EINTR, so I'm hesitant to muck with that.

Ah, it all makes sense now that I see the fallback strategy section of
the posix_fallocate() man page.  I was unaware that there were kernel
releases that had the syscall but lacked support in tmpfs.  Thanks for
testing and fixing that.

-- 
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] PATCH: pgbench - option to build using ppoll() for larger connection counts

[Rady, Doug]

On 9/25/17, 11:07, "Andres Freund"  wrote:

On 2017-09-25 18:01:40 +, Rady, Doug wrote:
> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
> when using ppoll(), the only connection limitation is system resources.

Hm, is there any need of using ppoll over poll?  IIRC it's a good bit
more common and there's, also iirc, a number of platforms with buggy
ppoll implementations.


Greetings,

Andres Freund


I used ppoll() as it can support the microseconds duration for \SLEEP meta 
command.

thanks!
doug
--
Doug Rady
Amazon Aurora PostgreSQL
radyd...@amazon.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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Thomas Munro]

On Tue, Sep 26, 2017 at 7:56 AM, Tom Lane  wrote:
> I wrote:
>> Hmm, so I tested this patch on my RHEL6 box (kernel 2.6.32) and it
>> immediately fell over with
>> 2017-09-25 14:23:48.410 EDT [325] FATAL:  could not resize shared memory 
>> segment "/PostgreSQL.1682054886" to 6928 bytes: Operation not supported
>> during startup.  I wonder whether we need to round off the request.
>
> Nope, rounding off doesn't help.  What does help is using posix_fallocate
> instead.  I surmise that glibc knows something we don't about how to call
> fallocate(2) successfully on this kernel version.
>
> Rather than dig into the guts of glibc to find that out, though, I think
> we should just s/fallocate/posix_fallocate/g on this patch.  The argument
> for using the former seemed pretty thin to begin with.

I didn't dig into the glibc source but my first guess is that
posix_fallocate() sees ENOTSUPP (from tmpfs on that vintage kernel)
and falls back to a write loop.  I thought I was doing enough by
testing for ENOSYS (based on the man page for fallocate which said
that should be expected on kernels < 2.6.23), but I see now that
ENOTSUPP is possible per-filesystem implementation and tmpfs support
was added some time after the 2.6.32 era:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2d12e22c59ce714008aa5266d769f8568d74eac

I'm not sure why glibc would provide a fallback for posix_fallocate()
but let ENOTSUPP escape from fallocate().

-- 
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Tom Lane]

I wrote:
> Rather than dig into the guts of glibc to find that out, though, I think
> we should just s/fallocate/posix_fallocate/g on this patch.  The argument
> for using the former seemed pretty thin to begin with.

Pushed with that change; we'll soon see what the buildfarm thinks.

I suspect that the provisions for EINTR and ENOSYS errors may now be
dead code, since the Linux man page for posix_fallocate mentions
neither.  They're not much code though, and POSIX itself *does*
list EINTR, so I'm hesitant to muck with 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] Built-in plugin for logical decoding output

[Petr Jelinek]

On 25/09/17 19:48, Joshua D. Drake wrote:
> On 09/25/2017 10:43 AM, Andres Freund wrote:
>> On 2017-09-25 10:38:52 -0700, Joshua D. Drake wrote:
>>> On 09/25/2017 10:32 AM, Petr Jelinek wrote:
 On 25/09/17 19:26, Tom Lane wrote:
> Alvaro Hernandez  writes:
>>>

 There is already about 3 million output plugins out there so I think we
 did reasonable job there. The fact that vast majority of that are
 various json ones gives reasonable hint that we should have that one in
 core though.
>>>
>>> And I am sure that 2ndQuadrant would be happy to add it to their
>>> version of
>>> Postgres and maintain it themselves.
>>>
>>> https://www.2ndquadrant.com/en/resources/2ndqpostgres/
>>
>> This doesn't seem like a good way to argue.
>>
> 
> Sorry, that wasn't supposed to be negative. My point was that
> 2ndQuadrant has a distribution of Postgres that they support. If
> 2ndQuadrant wants the feature, they could add it to their own without
> burdening the wider community further. It provides 2ndQuadrant what they
> are arguing for, benefits 2ndQuadrant as it increases the visibility and
> opportunity of their distribution for wider use.
> 

I am not sure I follow, we do have pglogical extension which works just
fine for this, thanks. You might have noticed that it's not what I was
suggesting we use for core.

-- 
  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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

[Peter Geoghegan]

On Mon, Sep 25, 2017 at 12:14 PM, Peter Geoghegan  wrote:
> But then our users categorically have to know about both formats,
> without any practical benefit to make up for it. You will also get
> people that don't realize that only one format is supported on some
> versions if go this way.

Oh, and if you go this way there is also a much bigger disadvantage:
you also break pg_restore when you cross the ICU 54 version boundary
in *either* direction (pg_collation.collname will be different, so
actually equivalent collations will not be recognized as such for
dump/restore purposes). This seems very likely, even, because ICU 54
isn't that old (we support versions as old as ICU 4.2), and because
you only have to have used ICU initdb collation for this to happen (no
CREATE COLLATION is necessary). Sure, you *may* be able to first do a
manual CREATE COLLATION when that happens, and that will be picked up
during the restore. But that's very user hostile.

That must have been the real reason why you canonicalized
pg_collation.collname (I doubt it had anything to do with how keyword
variants used to be created during initdb, as you suggested). As Tom
pointed out recently, we've actually always canonicalized collation
name for libc.

-- 
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] Row Level Security Documentation

[Dean Rasheed]

On 5 August 2017 at 10:03, Fabien COELHO  wrote:
> Patch applies cleanly, make html ok, new table looks good to me.
>

So I started looking at this patch, but before even considering the
new table proposed, I think there are multiple issues that need to be
resolved with the current docs:

Firstly, there are 4 separate places in the current CREATE POLICY docs
that say that multiple policies are combined using OR, which is of
course no longer correct, since PG10 added RESTRICTIVE policy support.
In fact, it wasn't even strictly correct in 9.5/9.6, because if
multiple different types of policy apply to a single command (e.g.
SELECT and UPDATE policies, being applied to an UPDATE command), then
they are combined using AND. Rather than trying to make this correct
in 4 different places, I think there should be a single new section of
the docs that explains how multiple policies are combined. This will
also reduce the number of places where the pre- and post-v10 docs
differ, making them easier to maintain.

In the 6th paragraph of the description section, the terminology used
is mixed up and does not match the terminology used elsewhere
("command" instead of "policy" and "policy" instead of "expression").

The description of UPDATE policies lists the commands that the policy
applies to (UPDATE and INSERT ... ON CONFLICT DO UPDATE), but fails to
mention SELECT FOR UPDATE and SELECT FOR SHARE.

The proposed patch distinguishes between UPDATE/DELETE commands that
have WHERE or RETURNING clauses, and those that don't, claiming that
the former require SELECT permissions and the latter don't. That's
based on a couple of statements from the current docs, but it's not
entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true
RETURNING now()" does not require SELECT permissions despite having
both WHERE and RETURNING clauses (OK, I admit that's a rather
contrived example, but still...), and conversely (a more realistic
example) "UPDATE foo SET a=b+c" does require SELECT permissions
despite not having WHERE or RETURNING clauses. I think we need to try
to be more precise about when SELECT permissions are required.

In the notes section, there is a note about there needing to be at
least one permissive policy for restrictive policies to take effect.
That's well worth pointing out, however, I fear that this note is
buried so far down the page (amongst some other very wordy notes) that
readers will miss it. I suggest moving it to the parameters section,
where permissive and restrictive policies are first introduced,
because it's a pretty crucial fact to be aware of when using these new
types of policy.

Attached is a proposed patch to address these issues, along with a few
other minor tidy-ups.

Regards,
Dean


create-policy-docs.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] Patch to address concerns about ICU collcollate stability in v10 (Was: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?)

[Peter Geoghegan]

On Mon, Sep 25, 2017 at 11:42 AM, Peter Eisentraut
 wrote:
> On 9/25/17 00:24, Peter Geoghegan wrote:
>> * Creates root collation as root-x-icu (collcollate "root"), not
>> und-x-icu. "und" means undefined language.
>
> I'm curious about this point.  "und" is defined in BCP 47.  I don't see
> "root" defined anywhere.  ICU converts the root collation to "und",
> AFAIK, so it seems to agree with the current naming.

In my patch, "root" is a string that is passed to get a language tag.
That's technically in the old format.

I think that this is another ICU vs. UCA/CLDR thing (this causes much
confusion). Note that "root" is mentioned in the ICU locale explorer,
for example: https://ssl.icu-project.org/icu-bin/locexp

Note also that ucol_open() comments/docs say this:

 * @param loc The locale containing the required collation rules.
 *Special values for locales can be passed in -
 *if NULL is passed for the locale, the default locale
 *collation rules will be used. If empty string ("") or
 *"root" are passed, UCA rules will be used.

I went with "root" because that produces a meaningful/useful display
name for pg_collation, and seems to be widely used elsewhere.

-- 
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] Built-in plugin for logical decoding output

[Christopher Browne]

On Sep 25, 2017 1:39 PM, "Joshua D. Drake"  wrote:

> On 09/25/2017 10:32 AM, Petr Jelinek wrote:
>
>> On 25/09/17 19:26, Tom Lane wrote:
>>
>>> Alvaro Hernandez  writes:
>>>
>>
>
>> There is already about 3 million output plugins out there so I think we
>> did reasonable job there. The fact that vast majority of that are
>> various json ones gives reasonable hint that we should have that one in
>> core though.
>>
>
> And I am sure that 2ndQuadrant would be happy to add it to their version
> of Postgres and maintain it themselves.
>
>
That's not the case to be thinking about...  Of course, 2Q can support some
extensions for their customers that use their binaries, that is not a
contraindication for having more in core.

I'd rather think based on questions like...  "Will that be supportable in
an ordinary Amazon instance?" Or "... In a Heroku instance?"

Those (Amazon and Heroku) are places that Slony won't run because it needs
a bit too much in the way of database superuser capabilities.

It's a very useful exercise to know which bits of this are easy to resolve
versus difficult versus irreconcilable.

Of course, we can't force everything into core, but given what IS in core,
it can look mighty dumb if...

a) we have neat and crucial features in core that are (validly!) trumpeted
in release notes, but

b) those features aren't usable without substantial out-of-core extensions
that many users cannot use.

Perhaps it's valid for logical replication to be considered out-of-scope
for generic Amazon/Heroku instances, but I'd prefer that to be the result
of a reasoned intent.

Re: [HACKERS] Reading backup label file for checkpoint and redo location during crash recovery

[Satyanarayana Narlapuram]

Thank you! Got it. 

-Original Message-
From: Stephen Frost [mailto:sfr...@snowman.net] 
Sent: Monday, September 25, 2017 10:57 AM
To: Magnus Hagander 
Cc: Satyanarayana Narlapuram ; 
PostgreSQL-development 
Subject: Re: [HACKERS] Reading backup label file for checkpoint and redo 
location during crash recovery

* Magnus Hagander (mag...@hagander.net) wrote:
> On Mon, Sep 25, 2017 at 7:43 PM, Stephen Frost  wrote:
> > * Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> > > During crash recovery, last checkpoint record information is 
> > > obtained
> > from the backup label if present, instead of getting it from the 
> > control file. This behavior is causing PostgreSQL database cluster 
> > not to come up until the backup label file is deleted (as the error message 
> > says).
> > >
> > > if (checkPoint.redo < checkPointLoc)
> > >   {
> > >  if (!ReadRecord(xlogreader,
> > checkPoint.redo, LOG, false))
> > > ereport(FATAL,
> > >   (errmsg("could 
> > > not
> > find redo location referenced by checkpoint record"),
> > >   errhint("If you 
> > > are
> > not restoring from a backup, try removing the file 
> > \"%s/backup_label\".", DataDir)));
> > >   }
> > >
> > > If we are recovering from a dump file, reading from the backup 
> > > label
> > files makes sense as the control file could be archived after a few 
> > checkpoints. But this is not the case for crash recovery, and is 
> > always safe to read the checkpoint record information from the control file.
> > > Is this behavior kept this way as there is no clear way to 
> > > distinguish
> > between the recovery from the dump and the regular crash recovery?
> >
> > This is why the exclusive backup method has been deprecated in PG10 
> > in favor of the non-exclusive backup method, which avoids this by 
> > not creating a backup label file (it's up to the backup software to 
> > store the necessary information and create the file for use during 
> > recovery).
> 
> Actally, it was deprecated already in 9.6, not just 10.

Whoops, right.  Thanks for the clarification. :)

Stephen


-- 
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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

[Peter Geoghegan]

On Mon, Sep 25, 2017 at 11:40 AM, Peter Eisentraut
 wrote:
> On 9/22/17 16:46, Peter Geoghegan wrote:
>> But you are *already* canonicalizing ICU collation names as BCP 47. My
>> point here is: Why not finish the job off, and *also* canonicalize
>> colcollate in the same way? This won't break ucol_open() if we take
>> appropriate precautions when we go to use the Postgres collation/ICU
>> locale.
>
> Reading over this code again, it is admittedly not quite clear why this
> "canonicalization" is in there right now.  I think it had something to
> do with how we built the keyword variants at one point.  It might not
> make sense.  I'd be glad to take that out and use the result straight
> from uloc_getAvailable() for collcollate.  That is, after all, the
> "canonical" version that ICU chooses to report to us.

But then our users categorically have to know about both formats,
without any practical benefit to make up for it. You will also get
people that don't realize that only one format is supported on some
versions if go this way.

>> One concern that makes me suggest this is: What happens when
>> the user *downgrades* ICU version, from a version where colcollate is
>> BCP 47 to one where it would not have been at initdb time? That will
>> break the downgrade in an unpleasant way, including in installations
>> that never do a CREATE COLLATION themselves. We want to be able to
>> restore a basebackup on a somewhat different OS, and have that still
>> work following REINDEX. At least, that seems like it should be an
>> important goal for us.
>
> This is an interesting point, and my proposal above would fix that.

I've already written a patch to standardize collcollate. If we do the
way you describe above instead, then what happens when ICU finally
removes the already deprecated legacy format?

> However, I think that taking a PostgreSQL data directory and moving or
> copying it to an *older* OS installation is always going to have a
> potential for problems.  So I wouldn't spend a huge amount of effort
> just to fix this specific case.

The downgrade thing is just the simplest, most immediate example of
where failing to standardize collcollate now could cause problems.

-- 
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] Built-in plugin for logical decoding output

[Magnus Hagander]

On Mon, Sep 25, 2017 at 8:20 PM, Alvaro Hernandez  wrote:

>
>
> On 25/09/17 20:18, Andres Freund wrote:
>
>> On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:
>>
>>>  However, if DMS uses it for what I'd call production use, I assume
>>> it is
>>> actually production quality. I bet they do enough testing, and don't ship
>>> software to potentially millions of customers if it doesn't work well.
>>> So...
>>> first, I'd consider this a a sign of robustness.
>>>
>> You've been in software for how long? ... ;)  There's quite mixed
>> experiences with DMS.
>>
>
> Actually long enough to understand that if someone "big" calls it
> production quality, we should not be pickier and assume it is --whether it
> is or not. People will accept it as such, and that's good enough.
>

Historically the fact that we have been pickier than many of the "someone
big":s is exactly how we ended up with the codebase and relative stability
we have today.

Just because someone is big doesn't mean they know what's right. In fact,
more often than not the opposite turns out to be true.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] Built-in plugin for logical decoding output

[Jignesh Shah]

On Mon, Sep 25, 2017 at 11:37 AM, Joshua D. Drake 
wrote:

> On 09/25/2017 11:31 AM, Alvaro Hernandez wrote:
>
>>
>>
> Whether or not they are included in a managed environment is generally
>>> based on two things:
>>>
>>> 1. Safety (why RDS doesn't allow certain C extensions)
>>> 2. Community/Popularity (Exactly why RDS has PostGIS)
>>> A. Demand with a prerequisite of #1
>>>
>>
>>  This is very clear. Now tell me: how many output plugins do you see
>> included in RDS. And in GCP's PostgreSQL? Azure Postgres? Heroku?
>>
>
> From RDS:
>
> Logical Replication for PostgreSQL on Amazon RDS
>
> Beginning with PostgreSQL version 9.4, PostgreSQL supports the streaming
> of WAL changes using logical replication slots. Amazon RDS supports logical
> replication for a PostgreSQL DB instance version 9.4.9 and higher and 9.5.4
> and higher. Using logical replication, you can set up logical replication
> slots on your instance and stream database changes through these slots to a
> client like pg_recvlogical. Logical slots are created at the database level
> and support replication connections to a single database.
>
> PostgreSQL logical replication on Amazon RDS is enabled by a new
> parameter, a new replication connection type, and a new security role. The
> client for the replication can be any client that is capable of
> establishing a replication connection to a database on a PostgreSQL DB
> instance.
>
> The most common clients for PostgreSQL logical replication are AWS
> Database Migration Service or a custom-managed host on an AWS EC2 instance.
> The logical replication slot knows nothing about the receiver of the
> stream; there is no requirement that the target be a replica database. Note
> that if you set up a logical replication slot and do not read from the
> slot, data can be written to your DB instance's storage and you can quickly
> fill up the storage on your instance.
>
> """
>
> I don't see why others wouldn't be available either. In fact, I am not
> sure why you couldn't use the JSON ones now. (Although I have not tested
> it).
>
> JD
>
>
>
>

Also to add, Amazon RDS for PostgreSQL does supports non-core plugins.
Wal2json output plugin for logical decoding is supported for versions 9.6.3+
and 9.5.7+  (link
)
.

Regards,
Jignesh

Re: [HACKERS] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Tom Lane]

I wrote:
> Hmm, so I tested this patch on my RHEL6 box (kernel 2.6.32) and it
> immediately fell over with
> 2017-09-25 14:23:48.410 EDT [325] FATAL:  could not resize shared memory 
> segment "/PostgreSQL.1682054886" to 6928 bytes: Operation not supported
> during startup.  I wonder whether we need to round off the request.

Nope, rounding off doesn't help.  What does help is using posix_fallocate
instead.  I surmise that glibc knows something we don't about how to call
fallocate(2) successfully on this kernel version.

Rather than dig into the guts of glibc to find that out, though, I think
we should just s/fallocate/posix_fallocate/g on this patch.  The argument
for using the former seemed pretty thin to begin with.

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] Fix number skipping in to_number

[Oliver Ford]

On Monday, 25 September 2017, Nathan Wagner  wrote:

> On Thu, Aug 17, 2017 at 12:33:02PM +0100, Oliver Ford wrote:
>
> > Ok I've made that change in the attached v3. I'm not sure as I'm on
> > en_US.UTF-8 locale too. Maybe something Windows specific?
>
> This patch applies against master (8485a25a), compiles, and
> passes a make check.
>
> I tested both on my mac laptop, and my linux server.
>
> If we want this patch, I'd say it's ready for committer.  We may want
> (and I can't believe I'm saying this) more discussion as to exactly what
> the strategy for to_number() (and friends) is.  Do we want to duplicate
> Oracle's functionality, or do we want a similar function to do similar
> things, without necessarily having a goal of identical behavior to
> oracle?
>
> For myself, I pretty much never use the to_date, to_number, or
> to_timestamp functions except when porting oracle code.  I do use the
> to_char functions on occasion.  If strftime were available, I probably
> wouldn't use them.
>
> I would commit this patch and update the TODO with a goal of making
> to_number as Oracle compatible as is reasonable.
>
>
Thanks for your review. The issue is that Oracle throws errors on many more
input cases than Postgres does, so making it exactly like Oracle could
break a lot of existing users. E.g. to_number ('123,000', '999') returns
'123' on Postgres, but throws an error on Oracle. So making it exactly
Oracle-like could break existing users who might rely on the current
behavior.

My view is that we shouldn't deliberately introduce errors in order to be
exactly like Oracle if we don't currently and there's a sane use case for
current behavior. Do you have any examples of results that are different
between Oracle and Postgres, and you think the Oracle result makes more
sense?

Re: [HACKERS] [PATCH] Generic type subscripting

[Oleg Bartunov]

On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut
 wrote:
> On 9/21/17 11:24, Dmitry Dolgov wrote:
>> One last thing that I need to clarify. Initially there was an idea to
>> minimize changes in `pg_type`
>
> I see, but there is no value in that if it makes everything else more
> complicated.

+1



>
> --
> Peter Eisentraut  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


-- 
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 #14825: enum type: unsafe use?

[Tom Lane]

Andrew Dunstan  writes:
> OK, that seems to be the consensus. So let's apply the blacklist patch
> and then separately remove the 'created in the same transaction' test.
> We'll need to adjust the regression tests and docs accordingly.

Agreed.  I'll work on that in a little bit.

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] Patch to address concerns about ICU collcollate stability in v10 (Was: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?)

[Peter Eisentraut]

On 9/25/17 00:24, Peter Geoghegan wrote:
> * Creates root collation as root-x-icu (collcollate "root"), not
> und-x-icu. "und" means undefined language.

I'm curious about this point.  "und" is defined in BCP 47.  I don't see
"root" defined anywhere.  ICU converts the root collation to "und",
AFAIK, so it seems to agree with the current naming.

-- 
Peter Eisentraut  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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

[Peter Eisentraut]

On 9/22/17 16:46, Peter Geoghegan wrote:
> But you are *already* canonicalizing ICU collation names as BCP 47. My
> point here is: Why not finish the job off, and *also* canonicalize
> colcollate in the same way? This won't break ucol_open() if we take
> appropriate precautions when we go to use the Postgres collation/ICU
> locale.

Reading over this code again, it is admittedly not quite clear why this
"canonicalization" is in there right now.  I think it had something to
do with how we built the keyword variants at one point.  It might not
make sense.  I'd be glad to take that out and use the result straight
from uloc_getAvailable() for collcollate.  That is, after all, the
"canonical" version that ICU chooses to report to us.

> One concern that makes me suggest this is: What happens when
> the user *downgrades* ICU version, from a version where colcollate is
> BCP 47 to one where it would not have been at initdb time? That will
> break the downgrade in an unpleasant way, including in installations
> that never do a CREATE COLLATION themselves. We want to be able to
> restore a basebackup on a somewhat different OS, and have that still
> work following REINDEX. At least, that seems like it should be an
> important goal for us.

This is an interesting point, and my proposal above would fix that.
However, I think that taking a PostgreSQL data directory and moving or
copying it to an *older* OS installation is always going to have a
potential for problems.  So I wouldn't spend a huge amount of effort
just to fix this specific case.

-- 
Peter Eisentraut  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] Built-in plugin for logical decoding output

[Andres Freund]

On 2017-09-25 21:31:11 +0300, Alvaro Hernandez wrote:
> > > > - Distribution and testing are non-trivial: many OS/archs combinations.
> > > > 
> > 
> > Yes, it is. Why would we want to increase that burden to this community?
> 
> 
>     That's a different story, and one I cannot argue against. If easying
> postgresql integration with other tools is not something of a priority or
> something the core group cannot add to all the stuff on their shoulders, all
> my due respect. PostgreSQL users will do without, someway, somehow. But IMHO
> this should be a really high priority, and saying that this would turn
> PostgreSQL into an Oracle code base is going too far ;)

Well, we can certainly use more help doing maintenance-y stuff, which
will in turn allow to get more stuff into core in a good state medium+
term... ;)

Less jokingly: I'm doubtful that the "not a priority" take is really
fair - there's a lot of priorities, and they compete for scant
resources. Which means people have to argue convincingly if they want to
add to that burden - just actually asking the question whether it's a
good use of resources doesn't mean it's not. Just that it's not yet
clear.


Greetings,

Andres Freund


-- 
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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 11:31 AM, Alvaro Hernandez wrote:




Whether or not they are included in a managed environment is generally 
based on two things:


1. Safety (why RDS doesn't allow certain C extensions)
2. Community/Popularity (Exactly why RDS has PostGIS)
    A. Demand with a prerequisite of #1


     This is very clear. Now tell me: how many output plugins do you see 
included in RDS. And in GCP's PostgreSQL? Azure Postgres? Heroku?


From RDS:

Logical Replication for PostgreSQL on Amazon RDS

Beginning with PostgreSQL version 9.4, PostgreSQL supports the streaming 
of WAL changes using logical replication slots. Amazon RDS supports 
logical replication for a PostgreSQL DB instance version 9.4.9 and 
higher and 9.5.4 and higher. Using logical replication, you can set up 
logical replication slots on your instance and stream database changes 
through these slots to a client like pg_recvlogical. Logical slots are 
created at the database level and support replication connections to a 
single database.


PostgreSQL logical replication on Amazon RDS is enabled by a new 
parameter, a new replication connection type, and a new security role. 
The client for the replication can be any client that is capable of 
establishing a replication connection to a database on a PostgreSQL DB 
instance.


The most common clients for PostgreSQL logical replication are AWS 
Database Migration Service or a custom-managed host on an AWS EC2 
instance. The logical replication slot knows nothing about the receiver 
of the stream; there is no requirement that the target be a replica 
database. Note that if you set up a logical replication slot and do not 
read from the slot, data can be written to your DB instance's storage 
and you can quickly fill up the storage on your instance.


"""

I don't see why others wouldn't be available either. In fact, I am not 
sure why you couldn't use the JSON ones now. (Although I have not tested 
it).


JD




--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


--
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] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 20:31, Joshua D. Drake wrote:

On 09/25/2017 10:19 AM, Petr Jelinek wrote:

On 25/09/17 18:48, Alvaro Hernandez wrote:





 In my opinion, logical decoding plugins that don't come with core
are close to worthless (don't get me wrong):



I respectfully disagree.


As do I.


    But Petr, without saying why, it does not help much to the 
discussion ;)







- They very unlikely will be installed in managed environments (an area
growing significantly).


Whether or not they are included in a managed environment is generally 
based on two things:


1. Safety (why RDS doesn't allow certain C extensions)
2. Community/Popularity (Exactly why RDS has PostGIS)
    A. Demand with a prerequisite of #1


    This is very clear. Now tell me: how many output plugins do you see 
included in RDS. And in GCP's PostgreSQL? Azure Postgres? Heroku?


    I'm looking at this from the practical perspective: if you would 
want to write a middleware for PostgreSQL that would rely on logical 
decoding, you definitely want to run on this platforms, or you are out 
of the game. If we want PostgreSQL to integrate more easily in nowadays 
very heterogeneous environments, this is key. And relying on 
non-included-or-acceptable-in-many environments output plugins is not, 
IMHO, a viable nor sensible option. I'd rather stick to test_decoding or 
pgoutput, no question.





- As anything that is not in core, raises concerns by users.


I find this a rather failing argument in today's market. If they are 
willing to migrate to Postgres, they are more than likely willing to 
use other open source software. Especially when combined with an 
expert telling them to.




- Distribution and testing are non-trivial: many OS/archs combinations.



Yes, it is. Why would we want to increase that burden to this community?



    That's a different story, and one I cannot argue against. If 
easying postgresql integration with other tools is not something of a 
priority or something the core group cannot add to all the stuff on 
their shoulders, all my due respect. PostgreSQL users will do without, 
someway, somehow. But IMHO this should be a really high priority, and 
saying that this would turn PostgreSQL into an Oracle code base is going 
too far ;)



    Álvaro

--

Alvaro Hernandez


---
OnGres



--
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Tom Lane]

Robert Haas  writes:
> On Mon, Sep 25, 2017 at 10:22 AM, Tom Lane  wrote:
>> I think we don't really have a lot of choice.  I propose applying this
>> as far back as 9.6 --- anyone think differently?

> +1.  If applies to 9.5 and 9.4 without a lot of work, I think we
> should apply it there as well, in case there is out-of-core use of DSM
> (or, in 9.5, also of ParallelContext).

Hmm, so I tested this patch on my RHEL6 box (kernel 2.6.32) and it
immediately fell over with

2017-09-25 14:23:48.410 EDT [325] FATAL:  could not resize shared memory 
segment "/PostgreSQL.1682054886" to 6928 bytes: Operation not supported

during startup.  I wonder whether we need to round off the request.

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] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 20:18, Andres Freund wrote:

On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:

     However, if DMS uses it for what I'd call production use, I assume it is
actually production quality. I bet they do enough testing, and don't ship
software to potentially millions of customers if it doesn't work well. So...
first, I'd consider this a a sign of robustness.

You've been in software for how long? ... ;)  There's quite mixed
experiences with DMS.


    Actually long enough to understand that if someone "big" calls it 
production quality, we should not be pickier and assume it is --whether 
it is or not. People will accept it as such, and that's good enough.


;)




FWIW, I don't think there's a huge problem w/ using test_decoding - the
output isn't pretty but it's parseable. It's too verbose due to
repeating column & type names (which also slows down), but...


    Everything is parseable. I don't have a big problem with that. 
Stability is another issue: as long as it supports high volume 
operations and doesn't break, it's acceptable enough.



    Álvaro


--

Alvaro Hernandez


---
OnGres



--
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 #14825: enum type: unsafe use?

[Andrew Dunstan]

On 09/25/2017 01:34 PM, David E. Wheeler wrote:
> On Sep 25, 2017, at 10:55, Andrew Dunstan  
> wrote:
>
>> Let's ask a couple of users who I think are or have been actually
>> hurting on this point. Christophe and David, any opinions?
> If I understand the issue correctly, I think I’d be fine with requiring ALTER 
> TYPE ADD LABEL to be disallowed in a transaction that also CREATEs the type 
> if it’s not currently possible to reliably tell when an enum was created in a 
> transaction. Once you can do that, then by all means allow it!
>


OK, that seems to be the consensus. So let's apply the blacklist patch
and then separately remove the 'created in the same transaction' test.
We'll need to adjust the regression tests and docs accordingly.

cheers

andrew

-- 
Andrew Dunstanhttps://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] PATCH: pgbench - option to build using ppoll() for larger connection counts

[Fabien COELHO]

Hello Again,


Two patches attached.
One based on REL9_6_STABLE.


I'd be surprise that there would be a backport unless there is a bug, so 
this one might not be useful.



One based on 'master' which can also apply to REL_10_STABLE.


Could you add your patches to the next CF?

--
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] PATCH: pgbench - option to build using ppoll() for larger connection counts

[Andres Freund]

On 2017-09-25 18:01:40 +, Rady, Doug wrote:
> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
> when using ppoll(), the only connection limitation is system resources.

Hm, is there any need of using ppoll over poll?  IIRC it's a good bit
more common and there's, also iirc, a number of platforms with buggy
ppoll implementations.


Greetings,

Andres Freund


-- 
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: pgbench - break out timing data for initialization phases

[Fabien COELHO]

Hello Doug,


total time: 316.03 s (insert 161.60 s, commit 0.64 s, vacuum 60.77 s, index 
93.01 s)


Definitely interesting.

There is a "ready for committers" patch in the CF which extensively rework 
the initialization: it becomes customizable, and this approach may not 
work as is after that...


Maybe you could investigate how it may be implemented on top of that?

Either show times when the phases are performed computed, or maybe use 
some auxiliary data structure to keep the information (hmmm...).


--
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] Enhancements to passwordcheck

[Bossart, Nathan]

Hi hackers,

Currently, the passwordcheck module provides a few basic checks to strengthen
passwords.  However, any configuration must be ready at compile time, and many
common password requirements cannot be enforced without creating a custom
version of this module.  I think there are a number of useful parameters that
could be added to enable common password restrictions, including the following
list, which is based on some asks from our customers:

passwordcheck.min_password_length
passwordcheck.min_uppercase_letters
passwordcheck.min_lowercase_letters
passwordcheck.min_numbers
passwordcheck.min_special_chars
passwordcheck.superuser_can_bypass
passwordcheck.max_expiry_period
passwordcheck.force_new_password

I'd like to use this thread to gauge community interest in adding this
functionality to this module.  If there is interest, I'll add it to the next
commitfest.

Nathan


-- 
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] Reading backup label file for checkpoint and redo location during crash recovery

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Mon, Sep 25, 2017 at 7:43 PM, Stephen Frost  wrote:
> > * Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> > > During crash recovery, last checkpoint record information is obtained
> > from the backup label if present, instead of getting it from the control
> > file. This behavior is causing PostgreSQL database cluster not to come up
> > until the backup label file is deleted (as the error message says).
> > >
> > > if (checkPoint.redo < checkPointLoc)
> > >   {
> > >  if (!ReadRecord(xlogreader,
> > checkPoint.redo, LOG, false))
> > > ereport(FATAL,
> > >   (errmsg("could not
> > find redo location referenced by checkpoint record"),
> > >   errhint("If you are
> > not restoring from a backup, try removing the file \"%s/backup_label\".",
> > DataDir)));
> > >   }
> > >
> > > If we are recovering from a dump file, reading from the backup label
> > files makes sense as the control file could be archived after a few
> > checkpoints. But this is not the case for crash recovery, and is always
> > safe to read the checkpoint record information from the control file.
> > > Is this behavior kept this way as there is no clear way to distinguish
> > between the recovery from the dump and the regular crash recovery?
> >
> > This is why the exclusive backup method has been deprecated in PG10 in
> > favor of the non-exclusive backup method, which avoids this by not
> > creating a backup label file (it's up to the backup software to store
> > the necessary information and create the file for use during recovery).
> 
> Actally, it was deprecated already in 9.6, not just 10.

Whoops, right.  Thanks for the clarification. :)

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Built-in plugin for logical decoding output

[Stephen Frost]

Andres, all,

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-09-25 19:32:29 +0200, Petr Jelinek wrote:
> > On 25/09/17 19:26, Tom Lane wrote:
> > > Alvaro Hernandez  writes:
> > >> In my opinion, logical decoding plugins that don't come with core 
> > >> are close to worthless (don't get me wrong):
> > > 
> > >> - They very unlikely will be installed in managed environments (an area 
> > >> growing significantly).
> > >> - As anything that is not in core, raises concerns by users.
> > >> - Distribution and testing are non-trivial: many OS/archs combinations.
> > > 
> > > The problem with this type of argument is that it leads directly to the
> > > conclusion that every feature users want must be in core.  We can't
> > > accept that conclusion, because we simply do not have the manpower or
> > > infrastructure to maintain a kitchen-sink, Oracle-sized code base.
> > > I think we're already more or less at the limit of the feature set we can
> > > deal with :-(.  The entire point of the output-plugin design was to allow
> > > useful replication stuff to be done outside of core; we need to make use
> > > of that.  (If that means we need better docs, then yeah, we'd better get
> > > that part done.)
> 
> I obvoiusly agree that not every possible output plugin should be put
> into core. A number of them have significant external dependencies
> and/or are specialized for a certain environment.
> 
> However I don't think that should mean that there's no possible output
> plugin that could/should be integrated into core. And I think Petr's
> right here:
> 
> > There is already about 3 million output plugins out there so I think we
> > did reasonable job there. The fact that vast majority of that are
> > various json ones gives reasonable hint that we should have that one in
> > core though.

I tend to agree with Andres & Petr here as well.  No, we certainly don't
want to add features that aren't going to be used much to core or which
stretch the resources we have beyond what we're able to handle, but
having a good, solid, output plugin that works well and can be built
upon should be included into core.  PG is often deployed in complex
ecosystems where we need to work with other systems and this is an
important part of that.

Also, to some extent, I'm hopeful that being both open to new features,
when they make sense, and providing more ways for other systems to
work with PG, will lead to more contributions and hopefully regular
contributors who can help us maintain the code base as it continues to
grow.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Built-in plugin for logical decoding output

[Andres Freund]

On 2017-09-25 13:50:29 -0400, Tom Lane wrote:
> Andres Freund  writes:
> >> On 25/09/17 19:26, Tom Lane wrote:
> >>> The problem with this type of argument is that it leads directly to the
> >>> conclusion that every feature users want must be in core.
> 
> > ... I don't think that should mean that there's no possible output
> > plugin that could/should be integrated into core.
> 
> Yeah, my point is just that the argument needs to be about why a
> *particular* plugin is valuable enough to justify adding it to the
> core developers' maintenance workload.

+1

Greetings,

Andres Freund


-- 
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] psql \d sequence display

[Fabien COELHO]

Hello,


This should be fixed for PG10, so if you have any feedback on the
design, please let me know soon.


Works for me on head against a 9.6 server, which is good.

My 0.02 €:

\d+ does not show more.

Maybe Type, Min, Max, Inc & Cycles are enough for \d?

The next/future or last/previous value is not shown. If one could be 
available it would be nice to have?


Maybe some names are a little large, eg "Increment" could be "Inc.".
The value is nearly always 1?

Not sure why it is "Cycles" (plural) instead of "Cycle".

The non regression test could also show a more esoteric sequence (cyclic, 
...).


There is no documentation. Maybe none is needed.

I do not understand why some queries have "... \n" "... \n" and others
have "\n ..." "\n ...". I would suggest to homogeneize around the former,
because "\nFROM ..." is less readable.

--
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] Reading backup label file for checkpoint and redo location during crash recovery

[Magnus Hagander]

On Mon, Sep 25, 2017 at 7:43 PM, Stephen Frost  wrote:

> Greetings Satya,
>
> * Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> > During crash recovery, last checkpoint record information is obtained
> from the backup label if present, instead of getting it from the control
> file. This behavior is causing PostgreSQL database cluster not to come up
> until the backup label file is deleted (as the error message says).
> >
> > if (checkPoint.redo < checkPointLoc)
> >   {
> >  if (!ReadRecord(xlogreader,
> checkPoint.redo, LOG, false))
> > ereport(FATAL,
> >   (errmsg("could not
> find redo location referenced by checkpoint record"),
> >   errhint("If you are
> not restoring from a backup, try removing the file \"%s/backup_label\".",
> DataDir)));
> >   }
> >
> > If we are recovering from a dump file, reading from the backup label
> files makes sense as the control file could be archived after a few
> checkpoints. But this is not the case for crash recovery, and is always
> safe to read the checkpoint record information from the control file.
> > Is this behavior kept this way as there is no clear way to distinguish
> between the recovery from the dump and the regular crash recovery?
>
> This is why the exclusive backup method has been deprecated in PG10 in
> favor of the non-exclusive backup method, which avoids this by not
> creating a backup label file (it's up to the backup software to store
> the necessary information and create the file for use during recovery).
>


Actally, it was deprecated already in 9.6, not just 10.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] logical replication and statistics

[Pavel Stehule]

2017-09-25 19:23 GMT+02:00 Petr Jelinek :

> On 25/09/17 19:19, Tom Lane wrote:
> > Pavel Stehule  writes:
> >> I had two instances on one server with different port. I am sure, so
> >> replication was functional. Only one issue is statistics
> >
> >> Master:
> >
> >> CREATE TABLE foo(id int primary key, a int);
> >> CREATE PUBLICATION test_pub FOR TABLE foo;
> >> INSERT INTO foo VALUES(1, 200);
> >
> >> slave
> >
> >> CREATE TABLE foo(id int primary key, a int);
> >> CREATE SUBSCRIPTION test_sub CONNECTION 'port=5432' PUBLICATION
> test_pub;
> >
> >> That was all
> >
> > In this example, nothing's been done yet by the actual replication
> > apply process, only by the initial table sync.  Maybe that accounts
> > for your not seeing stats?
> >
>
> The main replication worker should still be running though. The output
> of pg_stat_replication should only be empty if there is nothing running.
>
>
I did some inserts, updates, ..

I can recheck it - it was done on 10 RC

Pavel

--
>   Petr Jelinek  http://www.2ndQuadrant.com/
>   PostgreSQL Development, 24x7 Support, Training & Services
>

[HACKERS][BUG] Cache invalidation for queries that contains const of temporary composite type

[Maksim Milyutin]

Hello everyone!


I have found out the problem when try to sequentially call the function 
that casts constant to composite type of temporary table that is deleted 
ateach transaction termination (i.e. at each function call completion).



For example, we have the following function 'test':

CREATE OR REPLACE FUNCTION test()
 RETURNS void
 LANGUAGE plpgsql
AS $function$
begin
    create temp table tbl (id int) on commit drop;
    perform json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt;
end;
$function$


Оn the second and subsequent calls we'll observe the following error:

ERROR: cache lookup failed for type 16392


I investigated the problem and realized that result type of function 
*json_populate_record* (/funcresulttype/ field of /FuncExpr/ struct) as 
well as type of the first null argument (/consttype/ field of /Const/ 
struct) refer to the invalid composite type related with temporary 
table'tbl'. Namely they take a value of oid gotten from the first 'tbl' 
initialization. The plan of query *'perform 
json_populate_record(null::tbl, '{ "id": 11 }'::json) as tt'*is cached 
and is not invalidated at each function call. This is because** the 
statement of this query doesn't have any dependency from the 'tbl' 
relation (/relationOids/ list of /CachedPlanSource/ struct).



Attached patch resolves this problem by adding dependency from relation 
upon detection Const expression of composite type of that relation.


--
Regards,
Maksim Milyutin

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index b0c9e94..bf5e4f4 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -78,6 +78,12 @@ typedef struct
 	(((con)->consttype == REGCLASSOID || (con)->consttype == OIDOID) && \
 	 !(con)->constisnull)
 
+/*
+ * Check if a Const node has composite type
+ */
+#define ISTABLETYPECONST(con) \
+	(OidIsValid(get_typ_typrelid((con)->consttype)))
+
 #define fix_scan_list(root, lst, rtoffset) \
 	((List *) fix_scan_expr(root, (Node *) (lst), rtoffset))
 
@@ -1410,6 +1416,12 @@ fix_expr_common(PlannerInfo *root, Node *node)
 			root->glob->relationOids =
 lappend_oid(root->glob->relationOids,
 			DatumGetObjectId(con->constvalue));
+
+		/* Check whether const has composite type */
+		if (ISTABLETYPECONST(con))
+			root->glob->relationOids =
+lappend_oid(root->glob->relationOids,
+			get_typ_typrelid(con->consttype));
 	}
 	else if (IsA(node, GroupingFunc))
 	{
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index cb2a551..41be0d6 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -140,6 +140,21 @@ create temp sequence seq;
 
 execute p2;
 
+-- Check that invalidation deals with casting const value to temporary
+-- composite type reinitialized on each new transaction
+
+create function cache_query_with_composite_const() returns void as $$
+begin
+create temp table tbl(id int) on commit drop;
+
+-- Plan of the next query has to be rebuilt on each new call of function
+-- due to casting first argument 'null' to recreated temprary table 'tbl'
+perform json_populate_record(null::tbl, '{"id": 0}'::json);
+end$$ language plpgsql;
+
+select cache_query_with_composite_const();
+select cache_query_with_composite_const();
+
 -- Check DDL via SPI, immediately followed by SPI plan re-use
 -- (bug in original coding)
 

-- 
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] Built-in plugin for logical decoding output

[Tom Lane]

Andres Freund  writes:
>> On 25/09/17 19:26, Tom Lane wrote:
>>> The problem with this type of argument is that it leads directly to the
>>> conclusion that every feature users want must be in core.

> ... I don't think that should mean that there's no possible output
> plugin that could/should be integrated into core.

Yeah, my point is just that the argument needs to be about why a
*particular* plugin is valuable enough to justify adding it to the
core developers' maintenance workload.

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] Reading backup label file for checkpoint and redo location during crash recovery

[Andres Freund]

On 2017-09-25 13:43:32 -0400, Stephen Frost wrote:
> Greetings Satya,
> 
> * Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> > During crash recovery, last checkpoint record information is obtained from 
> > the backup label if present, instead of getting it from the control file. 
> > This behavior is causing PostgreSQL database cluster not to come up until 
> > the backup label file is deleted (as the error message says).
> > 
> > if (checkPoint.redo < checkPointLoc)
> >   {
> >  if (!ReadRecord(xlogreader, checkPoint.redo, 
> > LOG, false))
> > ereport(FATAL,
> >   (errmsg("could not find 
> > redo location referenced by checkpoint record"),
> >   errhint("If you are not 
> > restoring from a backup, try removing the file \"%s/backup_label\".", 
> > DataDir)));
> >   }
> > 
> > If we are recovering from a dump file, reading from the backup label files 
> > makes sense as the control file could be archived after a few checkpoints. 
> > But this is not the case for crash recovery, and is always safe to read the 
> > checkpoint record information from the control file.
> > Is this behavior kept this way as there is no clear way to distinguish 
> > between the recovery from the dump and the regular crash recovery?
> 
> This is why the exclusive backup method has been deprecated in PG10 in
> favor of the non-exclusive backup method, which avoids this by not
> creating a backup label file (it's up to the backup software to store
> the necessary information and create the file for use during recovery).
> 
> Please see:
> 
> https://www.postgresql.org/docs/10/static/continuous-archiving.html
> 
> In particular, section 25.3.3.

Might not be obvious for the more casual contributor:

And even before then, using pg_basebackup / the streaming replication
protocol version of creating a base backup, it was avoided.

Greetings,

Andres Freund


-- 
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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 10:43 AM, Andres Freund wrote:

On 2017-09-25 10:38:52 -0700, Joshua D. Drake wrote:

On 09/25/2017 10:32 AM, Petr Jelinek wrote:

On 25/09/17 19:26, Tom Lane wrote:

Alvaro Hernandez  writes:




There is already about 3 million output plugins out there so I think we
did reasonable job there. The fact that vast majority of that are
various json ones gives reasonable hint that we should have that one in
core though.


And I am sure that 2ndQuadrant would be happy to add it to their version of
Postgres and maintain it themselves.

https://www.2ndquadrant.com/en/resources/2ndqpostgres/


This doesn't seem like a good way to argue.



Sorry, that wasn't supposed to be negative. My point was that 
2ndQuadrant has a distribution of Postgres that they support. If 
2ndQuadrant wants the feature, they could add it to their own without 
burdening the wider community further. It provides 2ndQuadrant what they 
are arguing for, benefits 2ndQuadrant as it increases the visibility and 
opportunity of their distribution for wider use.


This is essentially what BigSQL and EDB are doing quite successfully. 
They add what the Core .Org community won't or doesn't (for whatever 
reason) and that makes their distribution attractive for their users.


Thanks,

JD


--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


--
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] Server crash due to SIGBUS(Bus Error) when trying to access the memory created using dsm_create().

[Tom Lane]

Robert Haas  writes:
> On Mon, Sep 25, 2017 at 10:22 AM, Tom Lane  wrote:
>> Thomas Munro  writes:
>>> So, do we want this patch?

>> I think we don't really have a lot of choice.  I propose applying this
>> as far back as 9.6 --- anyone think differently?

> +1.  If applies to 9.5 and 9.4 without a lot of work, I think we
> should apply it there as well, in case there is out-of-core use of DSM
> (or, in 9.5, also of ParallelContext).

I'll take a look, but I doubt it's worth much extra effort (and you
seem to agree).

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] Built-in plugin for logical decoding output

[Andres Freund]

On 2017-09-25 19:32:29 +0200, Petr Jelinek wrote:
> On 25/09/17 19:26, Tom Lane wrote:
> > Alvaro Hernandez  writes:
> >> In my opinion, logical decoding plugins that don't come with core 
> >> are close to worthless (don't get me wrong):
> > 
> >> - They very unlikely will be installed in managed environments (an area 
> >> growing significantly).
> >> - As anything that is not in core, raises concerns by users.
> >> - Distribution and testing are non-trivial: many OS/archs combinations.
> > 
> > The problem with this type of argument is that it leads directly to the
> > conclusion that every feature users want must be in core.  We can't
> > accept that conclusion, because we simply do not have the manpower or
> > infrastructure to maintain a kitchen-sink, Oracle-sized code base.
> > I think we're already more or less at the limit of the feature set we can
> > deal with :-(.  The entire point of the output-plugin design was to allow
> > useful replication stuff to be done outside of core; we need to make use
> > of that.  (If that means we need better docs, then yeah, we'd better get
> > that part done.)

I obvoiusly agree that not every possible output plugin should be put
into core. A number of them have significant external dependencies
and/or are specialized for a certain environment.

However I don't think that should mean that there's no possible output
plugin that could/should be integrated into core. And I think Petr's
right here:

> There is already about 3 million output plugins out there so I think we
> did reasonable job there. The fact that vast majority of that are
> various json ones gives reasonable hint that we should have that one in
> core though.


Greetings,

Andres Freund


-- 
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] Reading backup label file for checkpoint and redo location during crash recovery

[Stephen Frost]

Greetings Satya,

* Satyanarayana Narlapuram (satyanarayana.narlapu...@microsoft.com) wrote:
> During crash recovery, last checkpoint record information is obtained from 
> the backup label if present, instead of getting it from the control file. 
> This behavior is causing PostgreSQL database cluster not to come up until the 
> backup label file is deleted (as the error message says).
> 
> if (checkPoint.redo < checkPointLoc)
>   {
>  if (!ReadRecord(xlogreader, checkPoint.redo, 
> LOG, false))
> ereport(FATAL,
>   (errmsg("could not find 
> redo location referenced by checkpoint record"),
>   errhint("If you are not 
> restoring from a backup, try removing the file \"%s/backup_label\".", 
> DataDir)));
>   }
> 
> If we are recovering from a dump file, reading from the backup label files 
> makes sense as the control file could be archived after a few checkpoints. 
> But this is not the case for crash recovery, and is always safe to read the 
> checkpoint record information from the control file.
> Is this behavior kept this way as there is no clear way to distinguish 
> between the recovery from the dump and the regular crash recovery?

This is why the exclusive backup method has been deprecated in PG10 in
favor of the non-exclusive backup method, which avoids this by not
creating a backup label file (it's up to the backup software to store
the necessary information and create the file for use during recovery).

Please see:

https://www.postgresql.org/docs/10/static/continuous-archiving.html

In particular, section 25.3.3.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Built-in plugin for logical decoding output

[Andres Freund]

On 2017-09-25 10:38:52 -0700, Joshua D. Drake wrote:
> On 09/25/2017 10:32 AM, Petr Jelinek wrote:
> > On 25/09/17 19:26, Tom Lane wrote:
> > > Alvaro Hernandez  writes:
> 
> > 
> > There is already about 3 million output plugins out there so I think we
> > did reasonable job there. The fact that vast majority of that are
> > various json ones gives reasonable hint that we should have that one in
> > core though.
> 
> And I am sure that 2ndQuadrant would be happy to add it to their version of
> Postgres and maintain it themselves.
> 
> https://www.2ndquadrant.com/en/resources/2ndqpostgres/

This doesn't seem like a good way to argue.


-- 
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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 10:32 AM, Petr Jelinek wrote:

On 25/09/17 19:26, Tom Lane wrote:

Alvaro Hernandez  writes:




There is already about 3 million output plugins out there so I think we
did reasonable job there. The fact that vast majority of that are
various json ones gives reasonable hint that we should have that one in
core though.


And I am sure that 2ndQuadrant would be happy to add it to their version 
of Postgres and maintain it themselves.


https://www.2ndquadrant.com/en/resources/2ndqpostgres/

Thanks,

JD

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


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

[HACKERS] Reading backup label file for checkpoint and redo location during crash recovery

[Satyanarayana Narlapuram]

Hi there,

During crash recovery, last checkpoint record information is obtained from the 
backup label if present, instead of getting it from the control file. This 
behavior is causing PostgreSQL database cluster not to come up until the backup 
label file is deleted (as the error message says).

if (checkPoint.redo < checkPointLoc)
  {
 if (!ReadRecord(xlogreader, checkPoint.redo, LOG, 
false))
ereport(FATAL,
  (errmsg("could not find redo 
location referenced by checkpoint record"),
  errhint("If you are not 
restoring from a backup, try removing the file \"%s/backup_label\".", 
DataDir)));
  }

If we are recovering from a dump file, reading from the backup label files 
makes sense as the control file could be archived after a few checkpoints. But 
this is not the case for crash recovery, and is always safe to read the 
checkpoint record information from the control file.
Is this behavior kept this way as there is no clear way to distinguish between 
the recovery from the dump and the regular crash recovery?


Thanks,
Satya

Re: [HACKERS] [BUGS] BUG #14825: enum type: unsafe use?

[David E. Wheeler]

On Sep 25, 2017, at 10:55, Andrew Dunstan  
wrote:

> Let's ask a couple of users who I think are or have been actually
> hurting on this point. Christophe and David, any opinions?

If I understand the issue correctly, I think I’d be fine with requiring ALTER 
TYPE ADD LABEL to be disallowed in a transaction that also CREATEs the type if 
it’s not currently possible to reliably tell when an enum was created in a 
transaction. Once you can do that, then by all means allow it!

My $2.

Best,

David



signature.asc
Description: Message signed with OpenPGP

Re: [HACKERS] Built-in plugin for logical decoding output

[Petr Jelinek]

On 25/09/17 19:26, Tom Lane wrote:
> Alvaro Hernandez  writes:
>> In my opinion, logical decoding plugins that don't come with core 
>> are close to worthless (don't get me wrong):
> 
>> - They very unlikely will be installed in managed environments (an area 
>> growing significantly).
>> - As anything that is not in core, raises concerns by users.
>> - Distribution and testing are non-trivial: many OS/archs combinations.
> 
> The problem with this type of argument is that it leads directly to the
> conclusion that every feature users want must be in core.  We can't
> accept that conclusion, because we simply do not have the manpower or
> infrastructure to maintain a kitchen-sink, Oracle-sized code base.
> I think we're already more or less at the limit of the feature set we can
> deal with :-(.  The entire point of the output-plugin design was to allow
> useful replication stuff to be done outside of core; we need to make use
> of that.  (If that means we need better docs, then yeah, we'd better get
> that part done.)
> 

There is already about 3 million output plugins out there so I think we
did reasonable job there. The fact that vast majority of that are
various json ones gives reasonable hint that we should have that one in
core though.

-- 
  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 #14825: enum type: unsafe use?

[Christophe Pettus]

> On Sep 25, 2017, at 07:55, Andrew Dunstan  
> wrote:
> Let's ask a couple of users who I think are or have been actually
> hurting on this point. Christophe and David, any opinions?

Since about 90% of what I encounter in this area are automatically-generated 
migrations, having a clear set of (perhaps restrictive) rules which never fail 
is the most important.  It's easy to split the CREATE or ALTERs out into their 
own transaction, and leave usage (such as populating a table from a migration) 
to a second transaction.

It's not clear to me that this is a vote either way, but I think the easiest 
thing to explain ("you cannot use a new enum value in the same transaction that 
created it") is the best in this situation.

-- 
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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 10:19 AM, Petr Jelinek wrote:

On 25/09/17 18:48, Alvaro Hernandez wrote:





     In my opinion, logical decoding plugins that don't come with core
are close to worthless (don't get me wrong):



I respectfully disagree.


As do I.




- They very unlikely will be installed in managed environments (an area
growing significantly).


Whether or not they are included in a managed environment is generally 
based on two things:


1. Safety (why RDS doesn't allow certain C extensions)
2. Community/Popularity (Exactly why RDS has PostGIS)
A. Demand with a prerequisite of #1


- As anything that is not in core, raises concerns by users.


I find this a rather failing argument in today's market. If they are 
willing to migrate to Postgres, they are more than likely willing to use 
other open source software. Especially when combined with an expert 
telling them to.




- Distribution and testing are non-trivial: many OS/archs combinations.



Yes, it is. Why would we want to increase that burden to this community?


Thanks,

JD


--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


--
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] Built-in plugin for logical decoding output

[Tom Lane]

Alvaro Hernandez  writes:
> In my opinion, logical decoding plugins that don't come with core 
> are close to worthless (don't get me wrong):

> - They very unlikely will be installed in managed environments (an area 
> growing significantly).
> - As anything that is not in core, raises concerns by users.
> - Distribution and testing are non-trivial: many OS/archs combinations.

The problem with this type of argument is that it leads directly to the
conclusion that every feature users want must be in core.  We can't
accept that conclusion, because we simply do not have the manpower or
infrastructure to maintain a kitchen-sink, Oracle-sized code base.
I think we're already more or less at the limit of the feature set we can
deal with :-(.  The entire point of the output-plugin design was to allow
useful replication stuff to be done outside of core; we need to make use
of that.  (If that means we need better docs, then yeah, we'd better get
that part done.)

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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 10:15 AM, Gregory Brail wrote:
Yes. I'm advocating something "built-in" to Postgres. Any or all of 
those are likely a great starting point.


I don't see a benefit to having this "in postgres". The whole reason we 
have built out a mature and extensible product is so that not everything 
needs to be in postgres. We instead have a huge landscape of extensions 
created by our community that we don't have to manage in core.


Thanks,

JD



--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


--
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 replication and statistics

[Petr Jelinek]

On 25/09/17 19:19, Tom Lane wrote:
> Pavel Stehule  writes:
>> I had two instances on one server with different port. I am sure, so
>> replication was functional. Only one issue is statistics
> 
>> Master:
> 
>> CREATE TABLE foo(id int primary key, a int);
>> CREATE PUBLICATION test_pub FOR TABLE foo;
>> INSERT INTO foo VALUES(1, 200);
> 
>> slave
> 
>> CREATE TABLE foo(id int primary key, a int);
>> CREATE SUBSCRIPTION test_sub CONNECTION 'port=5432' PUBLICATION test_pub;
> 
>> That was all
> 
> In this example, nothing's been done yet by the actual replication
> apply process, only by the initial table sync.  Maybe that accounts
> for your not seeing stats?
> 

The main replication worker should still be running though. The output
of pg_stat_replication should only be empty if there is nothing running.

-- 
  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] Built-in plugin for logical decoding output

[Petr Jelinek]

On 25/09/17 18:48, Alvaro Hernandez wrote:
> 
> 
> On 25/09/17 19:39, Petr Jelinek wrote:
>>
>> Well, test_decoding is not meant for production use anyway, no need for
>> middleware to support it. The pgoutput is primarily used for internal
>> replication purposes, which is why we need something with more
>> interoperability in mind in the first place. The new plugin should still
>> support publications etc though IMHO.
>>
>>>  However, having said that, and while json is a great output format
>>> for interoperability, if there's a discussion on which plugin to include
>>> next, I'd also favor one that has some more compact representation
>>> format (or that supports several formats, not only json).
>>>
>> JSON is indeed great for interoperability, if you want more compact
>> format, use either pgoutput or write something of your own or do
>> conversion to something else in your consumer. I don't think postgres
>> needs to provide 100 different formats out of the box when there is an
>> API. The JSON output does not have to be extremely chatty either btw.
>>
> 
>     In my opinion, logical decoding plugins that don't come with core
> are close to worthless (don't get me wrong):
> 

I respectfully disagree.

> - They very unlikely will be installed in managed environments (an area
> growing significantly).
> - As anything that is not in core, raises concerns by users.
> - Distribution and testing are non-trivial: many OS/archs combinations.
> 
>     Given the above, I believe having a general-purpose output plugin
> in-core is critical to the use of logical decoding. As for 9.4-9.6 there
> is test_decoding, and given that AWS uses it for production, that's kind
> of fine. For 10 there is at least pgoutput, which could be used (even
> though it was meant for replication). But if a new plugin is to be
> developed for 11+, one really general purpose one, I'd say json is not a
> good choice if it is the only output it would support. json is too
> verbose, and replication, if anything, needs performance (it is both
> network heavy and serialization/deserialization is quite expensive). Why
> not, if one and only one plugin would be developed for 11+, general
> purpose, do something that is, indeed, more general, i.e., that supports
> high-performance scenarios too?
> 

IMO for general purpose use the adoption/ease of parsing is more
important criteria which is why JSON is good option. Again if you need
something more tuned to performance and you are fine with some hardship
in terms of parsing, what's stopping you from using pgoutput?

-- 
  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] Built-in plugin for logical decoding output

[Andres Freund]

Hi,

On 2017-09-25 12:56:00 -0400, Andrew Dunstan wrote:
> A general purpose lower bandwidth plugin might one supporting Protocol
> Buffers. The downside is that unlike json it's not self-contained, you
> need the message definitions to interpret the stream, AIUI.

I think that makes it a non-starter for many purposes were you care
about bandwidth.

Greetings,

Andres Freund


-- 
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 replication and statistics

[Tom Lane]

Pavel Stehule  writes:
> I had two instances on one server with different port. I am sure, so
> replication was functional. Only one issue is statistics

> Master:

> CREATE TABLE foo(id int primary key, a int);
> CREATE PUBLICATION test_pub FOR TABLE foo;
> INSERT INTO foo VALUES(1, 200);

> slave

> CREATE TABLE foo(id int primary key, a int);
> CREATE SUBSCRIPTION test_sub CONNECTION 'port=5432' PUBLICATION test_pub;

> That was all

In this example, nothing's been done yet by the actual replication
apply process, only by the initial table sync.  Maybe that accounts
for your not seeing stats?

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] Built-in plugin for logical decoding output

[Andres Freund]

On 2017-09-24 13:36:56 +0300, Alvaro Hernandez wrote:
>     However, if DMS uses it for what I'd call production use, I assume it is
> actually production quality. I bet they do enough testing, and don't ship
> software to potentially millions of customers if it doesn't work well. So...
> first, I'd consider this a a sign of robustness.

You've been in software for how long? ... ;)  There's quite mixed
experiences with DMS.

FWIW, I don't think there's a huge problem w/ using test_decoding - the
output isn't pretty but it's parseable. It's too verbose due to
repeating column & type names (which also slows down), but...


Greetings,

Andres Freund


-- 
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] Built-in plugin for logical decoding output

[Gregory Brail]

Yes. I'm advocating something "built-in" to Postgres. Any or all of those
are likely a great starting point.

As for protobuf, I'm a big advocate -- it is easy to use, fast, extensible,
runs on lots of platforms, and produces very compact output. However it
introduces a few dependencies to the build and that may make it too
difficult for wide options within Postgres.

On Mon, Sep 25, 2017 at 10:07 AM, Joshua D. Drake 
wrote:

> On 09/25/2017 09:59 AM, Gregory Brail wrote:
>
> However, I can't find any docs for the output format of pgoutput, which is
>> going to make it less likely for people to be able to consume it. Is anyone
>> working on docs? I know that it's a painful process.
>>
>> I also think that a JSON-format (or configurable format) plugin would
>> make this part of PG much more usable and I'd encourage the community to
>> come up with one.
>>
>
>
> https://github.com/ildus/decoder_json
> https://github.com/posix4e/jsoncdc
> https://github.com/leptonix/decoding-json
> https://github.com/Aloomaio/psql-json-decoder
>
> Thanks,
>
> JD
>
> --
> Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
>
> PostgreSQL Centered full stack support, consulting and development.
> Advocate: @amplifypostgres || Learn: https://pgconf.us
> * Unless otherwise stated, opinions are my own.   *
>

Re: [HACKERS] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 19:56, Andrew Dunstan wrote:


On 09/25/2017 12:48 PM, Alvaro Hernandez wrote:


On 25/09/17 19:39, Petr Jelinek wrote:

Well, test_decoding is not meant for production use anyway, no need for
middleware to support it. The pgoutput is primarily used for internal
replication purposes, which is why we need something with more
interoperability in mind in the first place. The new plugin should still
support publications etc though IMHO.


  However, having said that, and while json is a great output format
for interoperability, if there's a discussion on which plugin to
include
next, I'd also favor one that has some more compact representation
format (or that supports several formats, not only json).


JSON is indeed great for interoperability, if you want more compact
format, use either pgoutput or write something of your own or do
conversion to something else in your consumer. I don't think postgres
needs to provide 100 different formats out of the box when there is an
API. The JSON output does not have to be extremely chatty either btw.


     In my opinion, logical decoding plugins that don't come with core
are close to worthless (don't get me wrong):

- They very unlikely will be installed in managed environments (an
area growing significantly).
- As anything that is not in core, raises concerns by users.
- Distribution and testing are non-trivial: many OS/archs combinations.

     Given the above, I believe having a general-purpose output plugin
in-core is critical to the use of logical decoding. As for 9.4-9.6
there is test_decoding, and given that AWS uses it for production,
that's kind of fine. For 10 there is at least pgoutput, which could be
used (even though it was meant for replication). But if a new plugin
is to be developed for 11+, one really general purpose one, I'd say
json is not a good choice if it is the only output it would support.
json is too verbose, and replication, if anything, needs performance
(it is both network heavy and serialization/deserialization is quite
expensive). Why not, if one and only one plugin would be developed for
11+, general purpose, do something that is, indeed, more general,
i.e., that supports high-performance scenarios too?


   


A general purpose lower bandwidth plugin might one supporting Protocol
Buffers. The downside is that unlike json it's not self-contained, you
need the message definitions to interpret the stream, AIUI.


    Sure. But that's just a matter of documenting them, or even better, 
providing the .proto files, which are language-independent.


    There are also many other efficient serialization formats to 
explore, some self-contained, some not.



    Álvaro

--

Alvaro Hernandez


---
OnGres



--
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] Built-in plugin for logical decoding output

[Joshua D. Drake]

On 09/25/2017 09:59 AM, Gregory Brail wrote:

However, I can't find any docs for the output format of pgoutput, which 
is going to make it less likely for people to be able to consume it. Is 
anyone working on docs? I know that it's a painful process.


I also think that a JSON-format (or configurable format) plugin would 
make this part of PG much more usable and I'd encourage the community to 
come up with one.



https://github.com/ildus/decoder_json
https://github.com/posix4e/jsoncdc
https://github.com/leptonix/decoding-json
https://github.com/Aloomaio/psql-json-decoder

Thanks,

JD

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
* Unless otherwise stated, opinions are my own.   *


--
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] Shaky coding for vacuuming partitioned relations

[Bossart, Nathan]

On 9/24/17, 10:12 PM, "Michael Paquier"  wrote:
> Attached is a proposal of patch.

The patch seems reasonable to me, and I haven't encountered any issues in
my tests, even after applying the vacuum-multiple-relations patch on top
of it.

+* Take a lock here for the relation lookup. If ANALYZE or 
VACUUM spawn
+* multiple transactions, the lock taken here will be gone once 
the
+* current transaction running commits, which could cause the 
relation
+* to be gone, or the RangeVar might not refer to the OID 
looked up here.

I think this could be slightly misleading.  Perhaps it would be more
accurate to say that the lock will be gone any time vacuum() creates a new
transaction (either in vacuum_rel() or when use_own_xacts is true).

Nathan


-- 
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] Built-in plugin for logical decoding output

[Gregory Brail]

I'm encouraged that pgoutput exists and I'm sorry that I missed it before.
I think it's fine as a binary-only format. If someone can write a client
for the Postgres wire protocol as documented in Chapter 52 of the docs,
then they should have no trouble consuming the output from pgoutput.

However, I can't find any docs for the output format of pgoutput, which is
going to make it less likely for people to be able to consume it. Is anyone
working on docs? I know that it's a painful process.

I also think that a JSON-format (or configurable format) plugin would make
this part of PG much more usable and I'd encourage the community to come up
with one.

Finally, since there were some "why didn't you just" questions in the email
thread, let me write a little bit about what we were trying to do.

We have a set of data that represents the configuration of some of our
customer's systems. (This is for Apigee Edge, which is a software product
that represents a small part of Google Cloud, and which was developed long
before we joined Google.) We'd like to efficiently and reliably push
configuration changes down to our customer's systems, mostly to make it
possible for them to run parts of our software stack in their own data
centers, with limited or even unreliable network connectivity to the rest
of our services. Data replication is a great fit for this problem.

However, we want the downstream software components (the ones that our
customers run in their own data centers) to know when various things
change, we want those changes delivered in a consistent order, and we want
to be able to reliably receive them by having each consumer keep track of
where they currently are in the replication scheme. Logical replication is
a great fit for this because it enables us to build a list of all the
changes to this management data in a consistent order. Once we have that
list, it's fairly simple to persist it somewhere and let clients consume it
in various ways. (In our case, via an HTTP API that supports long polling.
Having all the clients consume a Kafka stream was not an option that we
wanted to consider.)

The difference between what we're trying to do and most solutions that use
logical replication is that we will have thousands or tens of thousands of
clients pulling a list of changes that originated in a single Postgres
database. That means that we need to index our own copy of the replication
output so that clients can efficiently get changes only to "their" data.
Furthermore, it means that we can't do things like create a unique
replication slot for each client. Instead, we have a smaller number of
servers that replicate from the master, and then those in turn give out
lists of changes to other clients.

On Mon, Sep 25, 2017 at 9:48 AM, Alvaro Hernandez  wrote:

>
>
> On 25/09/17 19:39, Petr Jelinek wrote:
>
>>
>> Well, test_decoding is not meant for production use anyway, no need for
>> middleware to support it. The pgoutput is primarily used for internal
>> replication purposes, which is why we need something with more
>> interoperability in mind in the first place. The new plugin should still
>> support publications etc though IMHO.
>>
>>  However, having said that, and while json is a great output format
>>> for interoperability, if there's a discussion on which plugin to include
>>> next, I'd also favor one that has some more compact representation
>>> format (or that supports several formats, not only json).
>>>
>>> JSON is indeed great for interoperability, if you want more compact
>> format, use either pgoutput or write something of your own or do
>> conversion to something else in your consumer. I don't think postgres
>> needs to provide 100 different formats out of the box when there is an
>> API. The JSON output does not have to be extremely chatty either btw.
>>
>>
> In my opinion, logical decoding plugins that don't come with core are
> close to worthless (don't get me wrong):
>
> - They very unlikely will be installed in managed environments (an area
> growing significantly).
> - As anything that is not in core, raises concerns by users.
> - Distribution and testing are non-trivial: many OS/archs combinations.
>
> Given the above, I believe having a general-purpose output plugin
> in-core is critical to the use of logical decoding. As for 9.4-9.6 there is
> test_decoding, and given that AWS uses it for production, that's kind of
> fine. For 10 there is at least pgoutput, which could be used (even though
> it was meant for replication). But if a new plugin is to be developed for
> 11+, one really general purpose one, I'd say json is not a good choice if
> it is the only output it would support. json is too verbose, and
> replication, if anything, needs performance (it is both network heavy and
> serialization/deserialization is quite expensive). Why not, if one and only
> one plugin would be developed for 11+, general purpose, do something that
> is, indeed, more general, i.e., that supports h

Re: [HACKERS] Built-in plugin for logical decoding output

[Andrew Dunstan]

On 09/25/2017 12:48 PM, Alvaro Hernandez wrote:
>
>
> On 25/09/17 19:39, Petr Jelinek wrote:
>>
>> Well, test_decoding is not meant for production use anyway, no need for
>> middleware to support it. The pgoutput is primarily used for internal
>> replication purposes, which is why we need something with more
>> interoperability in mind in the first place. The new plugin should still
>> support publications etc though IMHO.
>>
>>>  However, having said that, and while json is a great output format
>>> for interoperability, if there's a discussion on which plugin to
>>> include
>>> next, I'd also favor one that has some more compact representation
>>> format (or that supports several formats, not only json).
>>>
>> JSON is indeed great for interoperability, if you want more compact
>> format, use either pgoutput or write something of your own or do
>> conversion to something else in your consumer. I don't think postgres
>> needs to provide 100 different formats out of the box when there is an
>> API. The JSON output does not have to be extremely chatty either btw.
>>
>
>     In my opinion, logical decoding plugins that don't come with core
> are close to worthless (don't get me wrong):
>
> - They very unlikely will be installed in managed environments (an
> area growing significantly).
> - As anything that is not in core, raises concerns by users.
> - Distribution and testing are non-trivial: many OS/archs combinations.
>
>     Given the above, I believe having a general-purpose output plugin
> in-core is critical to the use of logical decoding. As for 9.4-9.6
> there is test_decoding, and given that AWS uses it for production,
> that's kind of fine. For 10 there is at least pgoutput, which could be
> used (even though it was meant for replication). But if a new plugin
> is to be developed for 11+, one really general purpose one, I'd say
> json is not a good choice if it is the only output it would support.
> json is too verbose, and replication, if anything, needs performance
> (it is both network heavy and serialization/deserialization is quite
> expensive). Why not, if one and only one plugin would be developed for
> 11+, general purpose, do something that is, indeed, more general,
> i.e., that supports high-performance scenarios too?
>
>
>   


A general purpose lower bandwidth plugin might one supporting Protocol
Buffers. The downside is that unlike json it's not self-contained, you
need the message definitions to interpret the stream, AIUI.

cheers

andrew

-- 
Andrew Dunstanhttps://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] Built-in plugin for logical decoding output

[Alvaro Hernandez]

On 25/09/17 19:39, Petr Jelinek wrote:


Well, test_decoding is not meant for production use anyway, no need for
middleware to support it. The pgoutput is primarily used for internal
replication purposes, which is why we need something with more
interoperability in mind in the first place. The new plugin should still
support publications etc though IMHO.


     However, having said that, and while json is a great output format
for interoperability, if there's a discussion on which plugin to include
next, I'd also favor one that has some more compact representation
format (or that supports several formats, not only json).


JSON is indeed great for interoperability, if you want more compact
format, use either pgoutput or write something of your own or do
conversion to something else in your consumer. I don't think postgres
needs to provide 100 different formats out of the box when there is an
API. The JSON output does not have to be extremely chatty either btw.



    In my opinion, logical decoding plugins that don't come with core 
are close to worthless (don't get me wrong):


- They very unlikely will be installed in managed environments (an area 
growing significantly).

- As anything that is not in core, raises concerns by users.
- Distribution and testing are non-trivial: many OS/archs combinations.

    Given the above, I believe having a general-purpose output plugin 
in-core is critical to the use of logical decoding. As for 9.4-9.6 there 
is test_decoding, and given that AWS uses it for production, that's kind 
of fine. For 10 there is at least pgoutput, which could be used (even 
though it was meant for replication). But if a new plugin is to be 
developed for 11+, one really general purpose one, I'd say json is not a 
good choice if it is the only output it would support. json is too 
verbose, and replication, if anything, needs performance (it is both 
network heavy and serialization/deserialization is quite expensive). Why 
not, if one and only one plugin would be developed for 11+, general 
purpose, do something that is, indeed, more general, i.e., that supports 
high-performance scenarios too?



    Álvaro

--

Alvaro Hernandez


---
OnGres



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