Re: [HACKERS] [sqlsmith] Unpinning error in parallel worker

[Thomas Munro]

On Mon, Mar 27, 2017 at 6:53 PM, Thomas Munro
 wrote:
> On Mon, Mar 27, 2017 at 8:38 AM, Thomas Munro
>  wrote:
>> On Mon, Mar 27, 2017 at 4:18 AM, Andreas Seltenreich  
>> wrote:
>>> Hi,
>>>
>>> today's testing with master as of d253b0f6e3 yielded two clusters that
>>> stopped processing queries.  Symptoms:
>>>
>>> [...]
>>
>> Thanks Andreas.  Investigating.
>
> First, the hanging stems from reentering dsm_backend_shutdown and
> trying to acquire DynamicSharedMemoryControlLock which we already
> acquired further down the stack in dsm_unpin_segment when it raised an
> error.  That's obviously not great, but the real question is how we
> reached this this-cannot-happen error condition.
>
> I reproduced this by inserting a sleep before dsa_attach_in_place,
> inserting a call to dsa_allocate into ExecInitParallelPlan so that the
> executor's DSA area owns at least one segment, and then cancelling a
> parallel query before the sleepy worker has managed to attach.  The
> DSA area is destroyed before the worker attaches, but the worker
> doesn't know this, and goes on to destroy it again after it learns
> that the query has been cancelled.
>
> In an earlier version of DSA, attaching should have failed in this
> scenario because the handle would be invalid.  Based on complaints
> about creating an extra DSM segment all the time even if we don't turn
> out to need it, I implemented "in place" DSA areas where the control
> object is in user-supplied shared memory, in this case in the parallel
> query main DSM segment.  But that created a new hazard: if you try to
> attach to a piece of memory that contains the remains of a
> already-destroyed DSA area, then we don't do anything to detect that.
> Oops.
>
> The attached patch fixes that one way: it detects refcnt == 0 as a
> defunct DSA area and raises an error when you try to attach.
>
> Another approach which I could explore would be to "reset" the DSA
> area instead of destroying it when the last backend detaches.  I'm not
> sure if that would ever be useful as a feature in its own right, but
> it would at least allow a very late worker to attach and then detach
> in an orderly fashion in this query-cancelled case, so you wouldn't
> get a different error message in the worker in this rare case.
>
> Thoughts?

Added to open items.

I considered whether the error message could be improved but it
matches the message for an existing similar case (where you try to
attach to an unknown handle).

The alternative approach I mentioned above doesn't seem warranted, as
you can already get various different failure messages depending on
timing.

Based on feedback on another thread about how to make reviewers' and
committers' jobs easier, here is a format-patch version with a short
description as raw material for a commit message, in case that is
helpful.

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


detect-late-dsa-attach-v2.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] Multiple false-positive warnings from Valgrind

[Kyotaro HORIGUCHI]

At Wed, 29 Mar 2017 12:34:52 +0900, Michael Paquier  
wrote in 
> On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier
>  wrote:
> > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
> >  wrote:
> >> Recently I've decided to run PostgreSQL under Valgrind according to wiki
> >> description [1]. Lots of warnings are generated [2] but it is my
> >> understanding that all of them are false-positive. For instance I've
> >> found these two reports particularly interesting:
> >>
> >> ```
> >> ==00:00:40:40.161 7677== Use of uninitialised value of size 8
> >> ==00:00:40:40.161 7677==at 0xA15FF5: pg_b64_encode (base64.c:68)
> >> ==00:00:40:40.161 7677==by 0x6FFE85: scram_build_verifier 
> >> (auth-scram.c:348)
...
> > I can see those warnings as well after calling a code path of
> > scram_build_verifier(), and I have a hard time seeing that as nothing
> > else than a false positive as you do. All those warnings go away if
> > you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before
> > calling pg_backend_random() but this data is filled in with
> > RAND_bytes() afterwards (if built with openssl). The estimated lengths
> > of the encoding are also correct. I don't see immediately what's wrong
> > here, this deserves a second look...
> 
> And it seems to me that this is caused by the routines of OpenSSL.
> When building without --with-openssl, using the fallback
> implementations of SHA256 and RAND_bytes I see no warnings generated
> by scram_build_verifier... I think it makes most sense to discard that
> from the list of open items.

FWIW a document of the function says that,

https://www.openssl.org/docs/man1.0.1/crypto/RAND_bytes.html

> The contents of buf is mixed into the entropy pool before
> retrieving the new pseudo-random bytes unless disabled at compile
> time (see FAQ).

This isn't saying that RAND_bytes does the same thing but
something similar can be happening there.

regards,

-- 
Kyotaro Horiguchi
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] [POC] A better way to expand hash indexes.

[Amit Kapila]

On Tue, Mar 28, 2017 at 8:56 PM, Robert Haas  wrote:
> On Tue, Mar 28, 2017 at 5:00 AM, Mithun Cy  wrote:
>>> This will go wrong for split point group zero.  In general, I feel if
>>> you handle computation for split groups lesser than
>>> SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE in the caller, then all your
>>> macros will become much simpler and less error prone.
>>
>> Fixed, apart from SPLITPOINT_PHASE_TO_SPLITPOINT_GRP rest all macros
>> only handle multi phase group. The SPLITPOINT_PHASE_TO_SPLITPOINT_GRP
>> is used in one more place at expand index so thought kepeping it as it
>> is is better.
>
> I wonder if we should consider increasing
> SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE somewhat.  For example, split
> point 4 is responsible for allocating only 16 new buckets = 128kB;
> doing those in four groups of two (16kB) seems fairly pointless.
> Suppose we start applying this technique beginning around splitpoint 9
> or 10.  Breaking 1024 new buckets * 8kB = 8MB of index growth into 4
> phases might save enough to be worthwhile.
>

10 sounds better point to start allocating in phases.

> Of course, even if we decide to apply this even for very small
> splitpoints, it probably doesn't cost us anything other than some
> space in the metapage.  But maybe saving space in the metapage isn't
> such a bad idea anyway.
>

Yeah metapage space is scarce, so lets try to save it as much possible.


Few other comments:
+/*
+ * This is just a trick to save a division operation. If you look into the
+ * bitmap of 0-based bucket_num 2nd and 3rd most significant bit will indicate
+ * which phase of allocation the bucket_num belongs to with in the group. This
+ * is because at every splitpoint group we allocate (2 ^ x) buckets and we have
+ * divided the allocation process into 4 equal phases. This macro returns value
+ * from 0 to 3.
+ */

+#define SPLITPOINT_PHASES_WITHIN_GROUP(sp_g, bucket_num) \
+ (((bucket_num) >> (sp_g - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE)) & \
+ SPLITPOINT_PHASE_MASK)

This won't work if we change SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE to
number other than 3.  I think you should change it so that it can work
with any value of  SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE.

I think you should name this define as SPLITPOINT_PHASE_WITHIN_GROUP
as this refers to only one particular phase within group.

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Fabien COELHO]

Hello Tom,


psql_if_on_error_stop... ok (test process exited with exit code 3)

Don't think we can have that.  Even if pg_regress considers it a success,
every hacker is going to have to learn that that's a "pass",


Well, it says "ok"...

and I don't think I want to be answering that question every week till 
kingdom come.


Hmmm.

What if the test is renamed, say "psql_if_exit_code_3"? Maybe the clue 
would be clear enough to avoid questions? Or just remove the exit message 
but check the exit code is as expected?



I'm not really sure we need a test for this behavior.


My 0.02€:

I have learned the hard way over the years that what is not tested does 
not really work, including error behaviors. These tests (well, the initial 
TAP version at least) helped debug the feature, and would help keeping it 
alive when the code is updated.


Now if you do not want this test, it can be removed. The feature is worthy 
even without it.


If we're sufficiently dead set on it, we could go back to the TAP-based 
approach,


Hmmm. You rejected it. I agree that TAP tests are not well suited for some 
simple tests because of their initdb overhead.


but I still doubt that this test is worth the amount of overhead that 
would add.


I think that there is an underlying issue with keeping on rejecting tests 
which aim at having a reasonable code coverage of features by exercising 
different paths.


Maybe there could be some "larger but still reasonable tests" activated on 
demand so as to being able to keep tests and run them from time to time, 
which would not interfere too much with committers' work, and that some 
farm animals would run?


I thought that was one of the purpose of TAP tests, but obviously it is 
not.


--
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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

[Rafia Sabih]

On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas  wrote:
> OK, but don't pg_event_trigger_dropped_objects and
> pg_event_trigger_ddl_commands need the same treatment?
>
Done.
I was only concentrating on the build farm failure cases, otherwise I
think more work might be required in this direction.

-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


system_defined_fn_update_v3.patch
Description: Binary data

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

[HACKERS] Fix obsolete comment in GetSnapshotData

[Craig Ringer]

Hi all

There's an outdated reference to GetOldestXmin(true, true) in
GetSnapshotData. It hasn't had that call signature for a long while
now. Update the comment to reflect the current signature.

diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index f32881b..4bf0243 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1556,7 +1556,8 @@ GetMaxSnapshotSubxidCount(void)
  *  older than this are known not running any more.
  *  RecentGlobalXmin: the global xmin (oldest TransactionXmin across all
  *  running transactions, except those running LAZY VACUUM).  This is
- *  the same computation done by GetOldestXmin(true, true).
+ *  the same computation done by
+ *  GetOldestXmin(NULL, PROCARRAY_FLAGS_DEFAULT|PROCARRAY_FLAGS_VACUUM)
  *  RecentGlobalDataXmin: the global xmin for non-catalog tables
  *  >= RecentGlobalXmin
  *


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


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

[HACKERS] show "aggressive" or not in autovacuum logs

[Kyotaro HORIGUCHI]

Hello, it would be too late but I'd like to propose this because
this cannot be back-patched.


In autovacuum logs, "%u skipped frozen" shows the number of pages
skipped by ALL_FROZEN only in aggressive vacuum.

So users cannot tell whether '0 skipped-frozen' means a
non-agressive vacuum or no frozen-pages in an agressive vacuum.

I think it is nice to have an indication whether the scan was
"agressive" or not in log output. Like this,

> LOG:  automatic aggressive vacuum of table 
> "template1.pg_catalog.pg_statistic": index scans: 0

"0 skipped frozen" is uesless in non-aggressive vacuum but
removing it would be too-much.  Inserting "aggressive" reduces
machine-readability so it might be better in another place. The
attached patch does the following.

>  LOG:  automatic vacuum of table "postgres.public.pgbench_branches": mode: 
> normal, index scans: 0
>  LOG:  automatic vacuum of table "postgres.public.pgbench_branches": mode: 
> aggressive, index scans: 0

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 5b43a66..644c93c 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -374,10 +374,11 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 			 * emitting individual parts of the message when not applicable.
 			 */
 			initStringInfo();
-			appendStringInfo(, _("automatic vacuum of table \"%s.%s.%s\": index scans: %d\n"),
+			appendStringInfo(, _("automatic vacuum of table \"%s.%s.%s\": mode: %s, index scans: %d\n"),
 			 get_database_name(MyDatabaseId),
 			 get_namespace_name(RelationGetNamespace(onerel)),
 			 RelationGetRelationName(onerel),
+			 aggressive ? "aggressive" : "normal",
 			 vacrelstats->num_index_scans);
 			appendStringInfo(, _("pages: %u removed, %u remain, %u skipped due to pins, %u skipped frozen\n"),
 			 vacrelstats->pages_removed,

-- 
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] Refactor handling of database attributes between pg_dump and pg_dumpall

[Haribabu Kommi]

On Tue, Mar 28, 2017 at 12:50 AM, Andreas Karlsson 
wrote:

> Hi,
>
> Here is my review. I agree with the goal of the refactoring, as we want to
> make it easier to dump all the properties for the database object. But I
> think we need to solve the issues with the special casing of postgres and
> template1 which I personally would find very surprising if pg_dump -C did.
> On the other hand I think that we cannot get away from having pg_dumpall
> give them a special treatment.
>

Thanks for the review.

I added a new option --enable-pgdumpall-behaviour to get the pg_dumpall
behaviour for the database objects
while dumping them through pg_dump. I am open to change the option name if
we come up with any other
better name.


> The nitpicking section is for minor code style errors.
>
> = Functional review
>
> I have not done an in depth functional review due to the discussion about
> how postgres and template1 should be handled.
>
> - The patch does not apply cleanly anymore
>
> - I do not like the change in behavior which causes "pg_dump -C postgres"
> to no longer include CREATE DATABASE. Special treatment of specific
> databases based on name makes sense in pg_dumpall, but not in pg_dump.
>

With the new additional option, CREATE DATABASE commands for postgres and
special treatment of
"SET default_transaction_read_only = off" still held.

- There are test failures in the pg_dump tests. It seems like some could be
> related to that you do not include CREATE DATABASE postgres in the dumps
> but I also get errors like 'ERROR:  syntax error at or near
> "fault_tablespace"'.
>
> not ok 691 - createdb: dumps CREATE DATABASE postgres
> not ok 3003 - pg_dumpall_dbprivs: dumps CREATE DATABASE dump_test
> not ok 11 - restore full dump using environment variables for connection
> parameters
> not ok 12 - no dump errors
> not ok 13 - restore full dump with command-line options for connection
> parameters
> not ok 14 - no dump errors
>

Fixed. Now all tests pass.

= Code review
>
> - As a response to "TBD -- is it necessary to get the default encoding": I
> think so, but either way changing this seems unrelated to this patch.
>

Removed.


> - I know it is taken from the old pg_dumpall code, but the way the
> database owner is handled seems I wrong.think we should set it like the
> owner for other objects. And more importantly it should respect --no-owner.
>

Removed the code for owner, as it is handled in another place with ALTER
DATABASE
command.


> - The logic for switching database when setting the default table space is
> broken. You generate "\ connect" rather than "\connect".
>

Fixed.



> - I saw the comment "Note that we do not support initial privileges
> (pg_init_privs) on databases." and wondered: why not? I definitly think
> that we should support this.
>

This is the existing code that moved from pg_dumpall.

= Nitpicking
>
> - You should probably use SGML style  over  and
>  for inline tags.
>

Corrected.


> - In "database-level properties such as Ownership, ACLs, [...]" I do not
> think that "Ownerships" shuld be capitalized.
>

Fixed.

- There are two extra spaces on the lines below, and a space is missing
> after the closing tag.
>
>  ALTER ROLE IN DATABASE ...  SET commands.
>
> with --create option to dump  ALTER ROLE IN DATABASE ...  SET
> 
>

Fixed.


> - On the following comment ".." should be "...", since that is the correct
> way to write an ellipsis.
>
> * Frame the ALTER .. SET .. commands and fill it in buf.
>

Fixed.


> - Rename arrayitem to configitem in makeAlterConfigCommand().
>

Corrected.


> - In makeAlterConfigCommand() you should do "*pos++ = '\0';" rather than
> "*pos = 0;" and then remove the later + 1 so our code matches with the code
> in dumpFunc(). Either is correct, but it would be nice if both pieces of
> code looked more similar.
>

Corrected.


> - You removed an empty line in pg_backup_utils.h between globals variables
> and function declartions which I think should be left there. It should be
> directly after g_verbose.


Fixed.

- There is something wrong with the indentation of the query for collecting
> info about databases in dumpDatabase() for PG >= 9.6.
>

Fixed.

- Missing space before "'' as rdatacl" in dumpDatabase(), and a missing
> space at the end of the string.
>

Fixed.


> - Double space in 'FROM pg_database  "' in dumpDatabase().


Fixed.

Updated patch attached.


Regards,
Hari Babu
Fujitsu Australia


pg_dump_changes_4.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] Reduce src/test/recovery verbosity

[Michael Paquier]

On Wed, Mar 29, 2017 at 12:37 PM, Craig Ringer  wrote:
> On 29 March 2017 at 08:57, Peter Eisentraut
>  wrote:
>> On 3/22/17 03:18, Craig Ringer wrote:
>>> Trivial patch to change 'diag' to 'note' in TAP tests in
>>> src/test/recovery attached.
>>>
>>> It'll reduce the test output a little.
>>
>> Committed, and also done the same in src/test/ssl/.
>
> Thanks. It won't do a ton of good unless we reduce the default TAP
> verbosity level too, but it's a start and will make things work better
> if/when we do that.

[hijack]
src/bin/pg_dump and src/test/modules/test_pgdump generate too much
output. If we could get tests to only print the final result, like how
many tests done and how many have passed, things would be much
friendlier.
[/hijack]
-- 
Michael


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

Re: [HACKERS] [PATCH] Reduce src/test/recovery verbosity

[Craig Ringer]

On 29 March 2017 at 08:57, Peter Eisentraut
 wrote:
> On 3/22/17 03:18, Craig Ringer wrote:
>> Trivial patch to change 'diag' to 'note' in TAP tests in
>> src/test/recovery attached.
>>
>> It'll reduce the test output a little.
>
> Committed, and also done the same in src/test/ssl/.

Thanks. It won't do a ton of good unless we reduce the default TAP
verbosity level too, but it's a start and will make things work better
if/when we do that.

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


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

Re: [HACKERS] On How To Shorten the Steep Learning Curve Towards PG Hacking...

[Craig Ringer]

On 29 March 2017 at 10:53, Amit Langote  wrote:
> Hi,
>
> On 2017/03/28 15:40, Kang Yuzhe wrote:
>> Thanks Tsunakawa for such an informative reply.
>>
>> Almost all of the docs related to the internals of PG are of introductory
>> concepts only.
>> There is even more useful PG internals site entitled "The Internals of
>> PostgreSQL" in http://www.interdb.jp/pg/ translation of the Japanese PG
>> Internals.
>>
>> The query processing framework that is described in the manual as you
>> mentioned is of informative and introductory nature.
>> In theory, the query processing framework described in the manual is
>> understandable.
>>
>> Unfortunate, it is another story to understand how query processing
>> framework in PG codebase really works.
>> It has become a difficult task for me to walk through the PG source code
>> for example how SELECT/INSERT/TRUNCATE in the the different modules under
>> "src/..". really works.
>>
>> I wish there were Hands-On with PostgreSQL Internals like
>> https://bkmjournal.wordpress.com/2017/01/22/hands-on-with-postgresql-internals/
>> for more complex PG features.
>>
>> For example, MERGE SQL standard is not supported yet by PG.  I wish there
>> were Hands-On with PostgreSQL Internals for MERGE/UPSERT. How it is
>> implemented in parser/executor/storage etc. modules with detailed
>> explanation for each code and debugging and other important concepts
>> related to system programming.
>
> I am not sure if I can show you that one place where you could learn all
> of that, but many people who started with PostgreSQL development at some
> point started by exploring the source code itself (either for learning or
> to write a feature patch), articles on PostgreSQL wiki, and many related
> presentations accessible using the Internet. I liked the following among
> many others:

Personally I have to agree that the learning curve is very steep. Some
of the docs and presentations help, but there's a LOT to understand.

When you're getting started you're lost in a world of language you
don't know, and trying to understand one piece often gets you lost in
other pieces. In no particular order:

* Memory contexts and palloc
* Managing transactions and how that interacts with memory contexts
and the default memory context
* Snapshots, snapshot push/pop, etc
* LWLocks, memory barriers, spinlocks, latches
* Heavyweight locks (and the different APIs to them)
* GUCs, their scopes, the rules around their callbacks, etc
* dynahash
* catalogs and oids and access methods
* The heap AM like heap_open
* relcache, catcache, syscache
* genam and the systable_ calls and their limitations with indexes
* The SPI
* When to use each of the above 4!
* Heap tuples and minimal tuples
* VARLENA
* GETSTRUCT, when you can/can't use it, other attribute fetching methods
* TOAST and detoasting datums.
* forming and deforming tuples
* LSNs, WAL/xlog generation and redo. Timelines. (ARGH, timelines).
* cache invalidations, when they can happen, and how to do anything
safely around them.
* TIDs, cmin and cmax, xmin and xmax
* postmaster, vacuum, bgwriter, checkpointer, startup process,
walsender, walreceiver, all our auxillary procs and what they do
* relmapper, relfilenodes vs relation oids, filenode extents
* ondisk structure, page headers, pages
* shmem management, buffers and buffer pins
* bgworkers
* PG_TRY() and PG_CATCH() and their limitations
* elog and ereport and errcontexts, exception unwinding/longjmp and
how it interacts with memory contexts, lwlocks, etc
* The nest of macros around datum manipulation and functions, PL
handlers. How to find the macros for the data types you want to work
with.
* Everything to do with the C API for arrays (is horrible)
* The details of the parse/rewrite/plan phases with rewrite calling
back into parse, paths, the mess with inheritance_planner, reading and
understanding plantrees
* The permissions and grants model and how to interact with it
* PGPROC, PGXACT, other main shmem structures
* Resource owners (which I still don't fully "get")
* Checkpoints, pg_control and ShmemVariableCache, crash recovery
* How globals are used in Pg and how they interact with fork()ing from
postmaster
* SSI (haven't gone there yet myself)
* 

Personally I recall finding the magic of resource owner and memory
context changing under me when I started/stopped xacts in a bgworker,
along with the need to manage snapshots and SPI state to be distinctly
confusing.

There are various READMEs, blog posts, presentation slides/videos, etc
that explain bits and pieces. But not much exists to tie it together
into a comprehensible hole with simple, minimal explanations for each
part so someone who's new to it all can begin to get a handle on it,
find resources to learn more about subsystems they need to care about,
etc.

Lots of it boils down to "read the code". But so much code! You don't
know if what you're reading is really relevant or if it's even
correct, or if it makes 

Re: [HACKERS] Schedule and Release Management Team for PG10

[Tom Lane]

Robert Haas  writes:
> On Thu, Mar 23, 2017 at 10:34 AM, Tom Lane  wrote:
>> As previously agreed at the PGCon 2016 developers meeting, we'll
>> institute v10 feature freeze at the completion of the current
>> commitfest (end of this month).

> This means, as I understand it, that no new features should be
> committed by anyone past the end of March, unless for some reason the
> RMT decides to grant an extension for some particular case.

Well, formally speaking, the RMT declares the onset of feature freeze.
I expressed an opinion that they'd probably do so at the end of March,
but it's their call as to exactly when to do so, and whether to grant
any limited extensions/exceptions.

My own thought is that there's room for at least a few days' slop in
the end date of the final commitfest, depending on what patches remain
open and what the prospects are for getting them done.  (In the past
we've sometimes let the final fest stretch on indefinitely, which is
clearly the Wrong Thing; but that doesn't mean that the Right Thing is
to say that it ends at 2017-04-01 00:00 UTC no matter what.)  The RMT
should look at things in another day or two and make a judgment call
about that.

> Instead,
> we should work on clearing out the open items list, so that we can go
> to beta in a timely fashion.

Certainly that should be the focus as soon as we are in feature freeze.

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] Multiple false-positive warnings from Valgrind

[Michael Paquier]

On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier
 wrote:
> On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
>  wrote:
>> Recently I've decided to run PostgreSQL under Valgrind according to wiki
>> description [1]. Lots of warnings are generated [2] but it is my
>> understanding that all of them are false-positive. For instance I've
>> found these two reports particularly interesting:
>>
>> ```
>> ==00:00:40:40.161 7677== Use of uninitialised value of size 8
>> ==00:00:40:40.161 7677==at 0xA15FF5: pg_b64_encode (base64.c:68)
>> ==00:00:40:40.161 7677==by 0x6FFE85: scram_build_verifier 
>> (auth-scram.c:348)
>> ==00:00:40:40.161 7677==by 0x6F3F76: encrypt_password (crypt.c:171)
>> ==00:00:40:40.161 7677==by 0x68F40C: CreateRole (user.c:403)
>> ==00:00:40:40.161 7677==by 0x85D53A: standard_ProcessUtility 
>> (utility.c:716)
>> ==00:00:40:40.161 7677==by 0x85CCC7: ProcessUtility (utility.c:353)
>> ==00:00:40:40.161 7677==by 0x85BD22: PortalRunUtility (pquery.c:1165)
>> ==00:00:40:40.161 7677==by 0x85BF20: PortalRunMulti (pquery.c:1308)
>> ==00:00:40:40.161 7677==by 0x85B4A0: PortalRun (pquery.c:788)
>> ==00:00:40:40.161 7677==by 0x855672: exec_simple_query (postgres.c:1101)
>> ==00:00:40:40.161 7677==by 0x8597BB: PostgresMain (postgres.c:4066)
>> ==00:00:40:40.161 7677==by 0x7C6322: BackendRun (postmaster.c:4317)
>> ==00:00:40:40.161 7677==  Uninitialised value was created by a stack 
>> allocation
>> ==00:00:40:40.161 7677==at 0x6FFDB7: scram_build_verifier 
>> (auth-scram.c:328)
>
> I can see those warnings as well after calling a code path of
> scram_build_verifier(), and I have a hard time seeing that as nothing
> else than a false positive as you do. All those warnings go away if
> you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before
> calling pg_backend_random() but this data is filled in with
> RAND_bytes() afterwards (if built with openssl). The estimated lengths
> of the encoding are also correct. I don't see immediately what's wrong
> here, this deserves a second look...

And it seems to me that this is caused by the routines of OpenSSL.
When building without --with-openssl, using the fallback
implementations of SHA256 and RAND_bytes I see no warnings generated
by scram_build_verifier... I think it makes most sense to discard that
from the list of open items.
-- 
Michael


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

Re: [HACKERS] postgres_fdw IMPORT SCHEMA and partitioned tables

[Michael Paquier]

On Wed, Mar 29, 2017 at 12:22 PM, Robert Haas  wrote:
> On Thu, Mar 9, 2017 at 8:47 PM, Amit Langote
>  wrote:
>>> It is not as straight-forward as it seems. A foreign table can be
>>> defined as a child (use of PARTITION OF), but not as a parent (use
>>> PARTITION BY), and IMPORT SCHEMA has to issue queries to create
>>> foreign tables. It seems to me that the correct fix here is to ignore
>>> child tables that are part of a partition, and just include the parent
>>> in what is imported so as when querying the parent through
>>> postgres_fdw all the child partitions are considered automatically.
>>> Thoughts?
>>
>> I think that makes sense.  The query in postgresImportForeignSchema() that
>> fetches the information about remote tables should be fixed to include
>> relkind = 'P' tables (partitioned tables) but exclude relispartition =
>> true (partitions).  Something like below:
>>
>> -   "WHERE c.relkind IN ('r', 'v', 'f', 'm') "
>> +   "WHERE c.relkind IN ('r', 'v', 'f', 'm', 'P') "
>> +   "  AND NOT c.relispartition "
>>
>> It means we don't import tables that are supposed to be partitions of some
>> table.  If we allow importing the latter, we get access to those
>> partitions anyway.
>>
>> I would like to hear more opinions of course.
>
> For the most part, I'm not very exercised about this either way.  I
> think that the above definition seems reasonably likely to be a useful
> one, but I certainly wouldn't try to insist on it in the face of
> opposition; I don't think it's 100% clear what users will want here.

Users like things that are friendly, and we are most likely going to
piss them off when using postgres_fdw if they need to list manually
each parent table from the IMPORT FOREIGN SCHEMA command.

> However, if we're going to do something about this, I think it should
> be done soon.  Otherwise, I'm going to advocate for reclassifying this
> issue from "open item" to "possible area for future development".

I was just waiting for the end of the CF before sending in a patch,
allocating now some time to look at some patches pending for reviews.
-- 
Michael


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

Re: [HACKERS] postgres_fdw IMPORT SCHEMA and partitioned tables

[Robert Haas]

On Thu, Mar 9, 2017 at 8:47 PM, Amit Langote
 wrote:
>> It is not as straight-forward as it seems. A foreign table can be
>> defined as a child (use of PARTITION OF), but not as a parent (use
>> PARTITION BY), and IMPORT SCHEMA has to issue queries to create
>> foreign tables. It seems to me that the correct fix here is to ignore
>> child tables that are part of a partition, and just include the parent
>> in what is imported so as when querying the parent through
>> postgres_fdw all the child partitions are considered automatically.
>> Thoughts?
>
> I think that makes sense.  The query in postgresImportForeignSchema() that
> fetches the information about remote tables should be fixed to include
> relkind = 'P' tables (partitioned tables) but exclude relispartition =
> true (partitions).  Something like below:
>
> -   "WHERE c.relkind IN ('r', 'v', 'f', 'm') "
> +   "WHERE c.relkind IN ('r', 'v', 'f', 'm', 'P') "
> +   "  AND NOT c.relispartition "
>
> It means we don't import tables that are supposed to be partitions of some
> table.  If we allow importing the latter, we get access to those
> partitions anyway.
>
> I would like to hear more opinions of course.

For the most part, I'm not very exercised about this either way.  I
think that the above definition seems reasonably likely to be a useful
one, but I certainly wouldn't try to insist on it in the face of
opposition; I don't think it's 100% clear what users will want here.

However, if we're going to do something about this, I think it should
be done soon.  Otherwise, I'm going to advocate for reclassifying this
issue from "open item" to "possible area for future development".

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


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

Re: [HACKERS] Partitioned tables and relfilenode

[Robert Haas]

On Mon, Mar 27, 2017 at 8:59 PM, Amit Langote
 wrote:
> Oops, my bad.  I will include it in the patch I'll send after addressing
> Robert's comments.  Thanks again!

That patch coming soon?

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


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

Re: [HACKERS] Schedule and Release Management Team for PG10

[Robert Haas]

On Thu, Mar 23, 2017 at 10:34 AM, Tom Lane  wrote:
> As previously agreed at the PGCon 2016 developers meeting, we'll
> institute v10 feature freeze at the completion of the current
> commitfest (end of this month).

This means, as I understand it, that no new features should be
committed by anyone past the end of March, unless for some reason the
RMT decides to grant an extension for some particular case.  Instead,
we should work on clearing out the open items list, so that we can go
to beta in a timely fashion.

https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items

If you have posted a bug fix patch for something committed to this
release, and it hasn't been committed yet, please a link to the thread
to the open items list.  If you have reported an issue with a v10
commit and it has not been fixed yet, please similarly add a link to
the open items list.  If, on the other hand, you committed a patch
that has generated one or more open items, please fix them.  Logical
replication is currently leading in the contest for "most unaddressed
open items" with 8, followed closely by SCRAM authentication with 6
and partitioning with 5.

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


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

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Tue, Mar 28, 2017 at 12:54 PM, Robert Haas  wrote:
> Regarding 0002, I think the parts that involve factoring out
> find_param_path_info() are uncontroversial.  Regarding the changes to
> adjust_appendrel_attrs(), my main question is whether we wouldn't be
> better off using an array representation rather than a List
> representation.  In other words, this function could take PlannerInfo
> *root, Node *node, int nappinfos, AppendRelInfo **appinfos.  Existing
> callers doing adjust_appendrel_attrs(root, whatever, appinfo) could
> just do adjust_appendrel_attrs(root, whatever, 1, ), not
> needing to allocate.  To make this work, adjust_child_relids() and
> find_appinfos_by_relids() would need to be adjusted to use a similar
> argument-passing convention.  I suspect this makes iterating over the
> AppendRelInfos mildly faster, too, apart from the memory savings.

Still regarding 0002, looking at adjust_appendrel_attrs_multilevel,
could we have a common code path for the baserel and joinrel cases?
It seems like perhaps you could just loop over root->append_rel_list.
For each appinfo, if (bms_is_member(appinfo->child_relid,
child_rel->relids)) bms_add_member(parent_relids,
appinfo->parent_relid).

This implementation would have some loss of efficiency in the
single-rel case because we'd scan all of the AppendRelInfos in the
list even if there's only one relid.  But you could fix that by
writing it like this:

foreach (lc, root->append_rel_list)
{
if (bms_is_member(appinfo->child_relid, child_rel->relids))
{
bms_add_member(parent_relids, appinfo->parent_relid);
if (child_rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
break;/* only one relid to find, and we've found it */
}
}
Assert(bms_num_members(child_rel->relids) == bms_num_members(parent_relids));

That seems pretty slick.  It is just as fast as the current
implementation for the single-rel case.  It allocates no memory
(unlike what you've got now).  And it handles the joinrel case using
essentially the same code as the simple rel case.

In 0003, it seems that it would be more consistent with what you did
elsewhere if the last argument to allow_star_schema_join were named
inner_paramrels rather than innerparams.  Other than that, I don't see
anything to complain about.

In 0004:

+   Assert(!rel->part_rels[cnt_parts]);
+   rel->part_rels[cnt_parts] = childrel;

break here?

+static void
+get_relation_partition_info(PlannerInfo *root, RelOptInfo *rel,
+   Relation
relation, bool inhparent)
+{
+   /* No partitioning information for an unpartitioned relation. */
+   if (relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
+   !inhparent ||

I still think the inhparent check should be moved to the caller.

In 0005:

+ * Returns a list of the RT indexes of the partitioned
child relations
+ * with any of joining relations' rti as the root parent RT index.

I found this wording confusing.  Maybe: Build and return a list
containing the RTI of every partitioned relation which is a child of
some rel included in the join.

+ * Note: Only call this function on joins between partitioned tables.

Or what, the boogeyman will come and get you?

(In other words, I don't think that's a very informative comment.)

I don't think 0011 is likely to be acceptable in current form.  I
can't imagine that we just went to the trouble of getting rid of
AppendRelInfos for child partitioned rels only to turn around and put
them back again.  If you just need the parent-child mappings, you can
get that from the PartitionedChildRelInfo list.

Unfortunately, I don't think we're likely to be able to get this whole
patch series into a committable form in the next few days, but I'd
like to keep reviewing it and working with you on it; there's always
next cycle.

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


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

Re: [HACKERS] crashes due to setting max_parallel_workers=0

[Tomas Vondra]

Hi,

On 03/28/2017 11:07 AM, Rushabh Lathia wrote:
... 

I think we all agree that we should get rid of nreaders from the 
GatherMergeState and need to do some code re-factor. But if I

understood correctly that Robert's concern was to do that re-factor
as separate commit.



Maybe. It depends on how valuable it's to keep Gather and GatherMerge 
similar - having nreaders in one and not the other seems a bit weird. 
But maybe the refactoring would remove it from both nodes?


Also, it does not really solve the issue that we're using 'nreaders' or 
'nworkers_launched' to access array with one extra element.



regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Forbid use of LF and CR characters in database and role names

[Michael Paquier]

On Wed, Feb 1, 2017 at 1:31 PM, Michael Paquier
 wrote:
> On Wed, Feb 1, 2017 at 1:31 PM, Michael Paquier
>  wrote:
>> On Tue, Nov 29, 2016 at 1:33 PM, Michael Paquier
>>  wrote:
>>> Patch moved to CF 2017-01.
>>
>> And nothing has happened since, the patch rotting a bit because of a
>> conflict in pg_dump's TAP tests. Attached is a rebased version.
>
> Moved to CF 2017-03..

And nothing has happened. This is the 4th commit fest where this patch
has been presented, and no committers have showed interest. I am
marking that as rejected, moving on to other things.
-- 
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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

[Amit Langote]

Hi,

On 2017/03/28 15:40, Kang Yuzhe wrote:
> Thanks Tsunakawa for such an informative reply.
> 
> Almost all of the docs related to the internals of PG are of introductory
> concepts only.
> There is even more useful PG internals site entitled "The Internals of
> PostgreSQL" in http://www.interdb.jp/pg/ translation of the Japanese PG
> Internals.
> 
> The query processing framework that is described in the manual as you
> mentioned is of informative and introductory nature.
> In theory, the query processing framework described in the manual is
> understandable.
> 
> Unfortunate, it is another story to understand how query processing
> framework in PG codebase really works.
> It has become a difficult task for me to walk through the PG source code
> for example how SELECT/INSERT/TRUNCATE in the the different modules under
> "src/..". really works.
> 
> I wish there were Hands-On with PostgreSQL Internals like
> https://bkmjournal.wordpress.com/2017/01/22/hands-on-with-postgresql-internals/
> for more complex PG features.
> 
> For example, MERGE SQL standard is not supported yet by PG.  I wish there
> were Hands-On with PostgreSQL Internals for MERGE/UPSERT. How it is
> implemented in parser/executor/storage etc. modules with detailed
> explanation for each code and debugging and other important concepts
> related to system programming.

I am not sure if I can show you that one place where you could learn all
of that, but many people who started with PostgreSQL development at some
point started by exploring the source code itself (either for learning or
to write a feature patch), articles on PostgreSQL wiki, and many related
presentations accessible using the Internet. I liked the following among
many others:

Introduction to Hacking PostgreSQL:
http://www.neilconway.org/talks/hacking/

Inside the PostgreSQL Query Optimizer:
http://www.neilconway.org/talks/optimizer/optimizer.pdf

Postgres Internals Presentations:
http://momjian.us/main/presentations/internals.html

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] Missing increment of vacrelstats->pinskipped_pages

[Kyotaro HORIGUCHI]

Hello,

At Tue, 28 Mar 2017 14:15:06 -0400, Jim Nasby  wrote in 
<61de5a80-2ffd-6b05-61e4-210fcb299...@nasby.net>
> lazy_vacuum_heap() does not count pages that it skips due to not
> obtaining the buffer cleanup lock. vacuum_pinskipped.patch fixes
> that. That should be backpatched to 9.5.
> 
> vacuum_comment.patch cleans up a comment in lazy_scan_heap().

> --- a/src/backend/commands/vacuumlazy.c
> +++ b/src/backend/commands/vacuumlazy.c
> @@ -1404,6 +1404,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats 
> *vacrelstats)
>   if (!ConditionalLockBufferForCleanup(buf))
>   {
>   ReleaseBuffer(buf);
> + vacrelstats->pinskipped_pages++;
>   ++tupindex;
>   continue;


This is within a dead-tuple removal loop so pinskipped pages
shouldn't be increment here. If several dead-tuples are in a
page, the page will be counted as many times as the number of the
tuples.

On the contrary, the page must not have been skipped since the
dead tuples are the result of a scan on the page.

The skipped *tuples* just remain dead and shown as "# dead row
versios cannot be removed yet".

regards,

-- 
Kyotaro Horiguchi
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] cast result of copyNode()

[Peter Eisentraut]

On 3/21/17 18:52, Mark Dilger wrote:
> The patch applies cleanly, compiles, and passes all the regression tests
> for me on my laptop.  Peter appears to have renamed the function copyObject
> as copyObjectImpl, which struct me as odd when I first saw it, but I don't 
> have
> a better name in mind, so that seems ok.

Committed that.

> If the purpose of this patch is to avoid casting so many things down to (Node 
> *),
> perhaps some additional work along the lines of the patch I'm attaching are
> appropriate.  (This patch applies on top Peter's v2 patch).  The idea being to
> keep objects as (Expr *) where appropriate, rather than casting through (Node 
> *)
> quite so much.

And that.

The distinction between Node and Expr is more theoretical and not
handled very ridigly throughout the code.  However, your patch seemed
like a gentle improvement in relatively new code, so it seems like a
good change.

-- 
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] Getting server crash after running sqlsmith

[Thomas Munro]

On Wed, Mar 29, 2017 at 2:23 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Mar 28, 2017 at 2:36 PM, Tom Lane  wrote:
>>> Hm ... I don't see a crash here, but I wonder whether you have parameters
>>> set that would cause this query to be run as a parallel query?  Because
>>> pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
>>> probably insane.
>
>> /me blinks
>
>> Uh, what's insane about that?  All it does is test a GUC (which is
>> surely parallel-safe) and call SendPostmasterSignal (which seems safe,
>> too).
>
> Well, if you don't like that theory, what's yours?

I can't reproduce this either.  But here's a theory: this query
signals the postmaster repeatedly fast, and with just the right kind
of difficulty scheduling/waking to the postmaster to deliver the
signal on an overloaded machine, maybe there is always a new SIGUSR1
and PMSIGNAL_ROTATE_LOGFILE waiting once the signal handler reaches
PG_SETMASK(), at which point it immediately recurses into
the signal handler until it blows the stack.

-- 
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] Getting server crash after running sqlsmith

[Robert Haas]

On Tue, Mar 28, 2017 at 9:23 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Mar 28, 2017 at 2:36 PM, Tom Lane  wrote:
>>> Hm ... I don't see a crash here, but I wonder whether you have parameters
>>> set that would cause this query to be run as a parallel query?  Because
>>> pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
>>> probably insane.
>
>> /me blinks
>
>> Uh, what's insane about that?  All it does is test a GUC (which is
>> surely parallel-safe) and call SendPostmasterSignal (which seems safe,
>> too).
>
> Well, if you don't like that theory, what's yours?

Gremlins?

The stack trace seems to show that the process is receiving SIGUSR1 at
a very high rate.  Every time sigusr1_handler() reaches
PG_SETMASK(), it immediately gets a SIGUSR1 and jumps back
into sigusr1_handler().  Now, this seems like a design flaw in
sigusr1_handler().  Likely the operating system blocks SIGUSR1 on
entry to the signal handler so that it's not possible for a high rate
of signal delivery to blow out the stack, but we forcibly unblock it
before returning, thus exposing ourselves to blowing out the stack.
And we have, apparently, no stack depth check here nor any other way
of preventing the infinite recursion.

I imagine here the behavior is platform-dependent, but I'd guess that
select pg_current_logfile() from generate_series(1,100) g might
reproduce this on affected platforms with or without parallel query in
the mix.  It looks like we've conveniently provided both a function
that can be used to SIGUSR1 the heck out of the postmaster and a
postmaster that is, at least on such platforms, vulnerable to crashing
if you do that.

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


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

Re: [HACKERS] Vacuum: allow usage of more than 1GB of work mem

[Claudio Freire]

On Wed, Feb 1, 2017 at 7:55 PM, Claudio Freire  wrote:
> On Wed, Feb 1, 2017 at 6:13 PM, Masahiko Sawada  wrote:
>> On Wed, Feb 1, 2017 at 10:02 PM, Claudio Freire  
>> wrote:
>>> On Wed, Feb 1, 2017 at 5:47 PM, Masahiko Sawada  
>>> wrote:
 Thank you for updating the patch.

 Whole patch looks good to me except for the following one comment.
 This is the final comment from me.

 /*
  *  lazy_tid_reaped() -- is a particular tid deletable?
  *
  *  This has the right signature to be an IndexBulkDeleteCallback.
  *
  *  Assumes dead_tuples array is in sorted order.
  */
 static bool
 lazy_tid_reaped(ItemPointer itemptr, void *state)
 {
 LVRelStats *vacrelstats = (LVRelStats *) state;

 You might want to update the comment of lazy_tid_reaped() as well.
>>>
>>> I don't see the mismatch with reality there (if you consider
>>> "dead_tples array" in the proper context, that is, the multiarray).
>>>
>>> What in particular do you find out of sync there?
>>
>> The current lazy_tid_reaped just find a tid from a tid array using
>> bsearch but in your patch lazy_tid_reaped handles multiple tid arrays
>> and processing method become complicated. So I thought it's better to
>> add the description of this function.
>
> Alright, updated with some more remarks that seemed relevant

I just realized I never updated the early free patch after the
multiarray version.

So attached is a patch that frees the multiarray as early as possible
(just after finishing with index bulk deletes, right before doing
index cleanup and attempting truncation).

This should make the possibly big amount of memory available to other
processes for the duration of those tasks, which could be a long time
in some cases.
From f080f8377b7200ae9c2a02abfb0ef0bf6e2d5d69 Mon Sep 17 00:00:00 2001
From: Claudio Freire 
Date: Tue, 28 Mar 2017 22:40:39 -0300
Subject: [PATCH] Vacuum: free dead tuples array as early as possible

Allow other parts of the system to benefit from the possibly
large amount of memory reserved for dead tuples after they're
no longer necessary.

While the memory would be freed when the command finishes, it
can sometimes be some considerable time between the time vacuum
is done with the array until the command finishes - mostly due
to truncate taking a long time.
---
 src/backend/commands/vacuumlazy.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 4a6895c..60a6c18 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -206,6 +206,7 @@ static void lazy_space_alloc(LVRelStats *vacrelstats, BlockNumber relblocks);
 static void lazy_record_dead_tuple(LVRelStats *vacrelstats,
 	   ItemPointer itemptr);
 static void lazy_clear_dead_tuples(LVRelStats *vacrelstats);
+static void lazy_free_dead_tuples(LVRelStats *vacrelstats);
 static bool lazy_tid_reaped(ItemPointer itemptr, void *state);
 static inline int vac_cmp_itemptr(const void *left, const void *right);
 static bool heap_page_is_all_visible(Relation rel, Buffer buf,
@@ -1358,6 +1359,9 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
 	pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
  PROGRESS_VACUUM_PHASE_INDEX_CLEANUP);
 
+	/* dead tuples no longer needed past this point */
+	lazy_free_dead_tuples(vacrelstats);
+
 	/* Do post-vacuum cleanup and statistics update for each index */
 	for (i = 0; i < nindexes; i++)
 		lazy_cleanup_index(Irel[i], indstats[i], vacrelstats);
@@ -2165,6 +2169,24 @@ lazy_clear_dead_tuples(LVRelStats *vacrelstats)
 }
 
 /*
+ * lazy_free_dead_tuples - reset all dead tuples segments
+ * and free all allocated memory
+ */
+static void
+lazy_free_dead_tuples(LVRelStats *vacrelstats)
+{
+	int			nseg;
+
+	for (nseg = 0; nseg < vacrelstats->dead_tuples.num_segs; nseg++)
+		pfree(vacrelstats->dead_tuples.dt_segments[nseg].dt_tids);
+	pfree(vacrelstats->dead_tuples.dt_segments);
+	vacrelstats->dead_tuples.last_seg = 0;
+	vacrelstats->dead_tuples.num_segs = 0;
+	vacrelstats->dead_tuples.num_entries = 0;
+	vacrelstats->dead_tuples.dt_segments = NULL;
+}
+
+/*
  *	lazy_tid_reaped() -- is a particular tid deletable?
  *
  *		This has the right signature to be an IndexBulkDeleteCallback.
-- 
1.8.4.5


-- 
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] free space map and visibility map

[Kyotaro HORIGUCHI]

Hello,

At Tue, 28 Mar 2017 08:50:58 -0700, Jeff Janes  wrote in 

Re: [HACKERS] Getting server crash after running sqlsmith

[Tom Lane]

Robert Haas  writes:
> On Tue, Mar 28, 2017 at 2:36 PM, Tom Lane  wrote:
>> Hm ... I don't see a crash here, but I wonder whether you have parameters
>> set that would cause this query to be run as a parallel query?  Because
>> pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
>> probably insane.

> /me blinks

> Uh, what's insane about that?  All it does is test a GUC (which is
> surely parallel-safe) and call SendPostmasterSignal (which seems safe,
> too).

Well, if you don't like that theory, what's yours?

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] make check-world output

[Peter Eisentraut]

On 3/10/17 19:15, Jeff Janes wrote:
> Should --enable-tap-tests be mentioned in "32.1.3. Additional Test
> Suites"?   Or at least cross-referenced from "32.4. TAP Tests"?  

Done.

-- 
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] [COMMITTERS] pgsql: Improve performance of find_all_inheritors()

[Robert Haas]

On Tue, Mar 28, 2017 at 3:59 PM, Aleksander Alekseev
 wrote:
> Hi, Robert.
>
>> I'm a little worried that this will be noticeably slower when the
>> number of partitions is small, like 4 or 8 or 16.  In other places,
>> we've found it necessary to support both a list-based strategy and a
>> hash-based strategy to avoid regressing small cases (e.g.
>> join_rel_list + join_rel_hash in PlannerInfo, ResourceArray in
>> resowner.c, etc).
>
> I've checked it [1]. Apparently in this particular case patches make code
> faster on small amount of partitions as well.

Huh.  Well, in that case, cool.

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


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

Re: [HACKERS] Getting server crash after running sqlsmith

[Robert Haas]

On Tue, Mar 28, 2017 at 2:36 PM, Tom Lane  wrote:
> tushar  writes:
>> After runinng sqlsmith against latest sources of PG v10  , able to see a
>> crash -
>
> Hm ... I don't see a crash here, but I wonder whether you have parameters
> set that would cause this query to be run as a parallel query?  Because
> pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
> probably insane.

/me blinks

Uh, what's insane about that?  All it does is test a GUC (which is
surely parallel-safe) and call SendPostmasterSignal (which seems safe,
too).

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


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

Re: [HACKERS] make check-world output

[Peter Eisentraut]

On 3/13/17 05:35, Dagfinn Ilmari Mannsåker wrote:
> Another thing I noticed is that there's a bunch of 'diag' calls in the
> tests scripts (particularly ssl/t/001_ssltests.pl and
> recovery/t/001_stream_rep.pl) that should probably be 'note's instead,
> so they don't pollute STDERR in non-verbose mode.  'diag' should only be
> used to output extra diagnostics in the case of test failures, 'note' is
> for test progress/status updates.

This has been fixed.

-- 
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] [PATCH] Reduce src/test/recovery verbosity

[Peter Eisentraut]

On 3/22/17 03:18, Craig Ringer wrote:
> Trivial patch to change 'diag' to 'note' in TAP tests in
> src/test/recovery attached.
> 
> It'll reduce the test output a little.

Committed, and also done the same in src/test/ssl/.

-- 
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] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Tom Lane]

Corey Huinker  writes:
[ 0001-psql-if-v28.patch ]

Starting to look at this version, and what jumped out at me in testing
is that the regression output looks like this:

parallel group (12 tests):  psql_if_on_error_stop dbsize async misc_functions 
tidscan alter_operator tsrf psql alter_generic misc stats_ext sysviews
 alter_generic... ok
 alter_operator   ... ok
 misc ... ok
 psql ... ok
 psql_if_on_error_stop... ok (test process exited with exit code 3)
 async... ok
 dbsize   ... ok
 misc_functions   ... ok
 sysviews ... ok
 tsrf ... ok
 tidscan  ... ok
 stats_ext... ok

Don't think we can have that.  Even if pg_regress considers it a success,
every hacker is going to have to learn that that's a "pass", and I don't
think I want to be answering that question every week till kingdom come.

I'm not really sure we need a test for this behavior.  If we're
sufficiently dead set on it, we could go back to the TAP-based approach,
but I still doubt that this test is worth the amount of overhead that
would add.

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] logical decoding of two-phase transactions

[Craig Ringer]

On 28 Mar. 2017 23:08, "Andres Freund"  wrote:

>
> > >> I don't think its for us to say what the plugin is allowed to do. We
> > >> decided on a plugin architecture, so we have to trust that the plugin
> > >> author resolves the issues. We can document them so those choices are
> > >> clear.
> > >
> > > I don't think this is "plugin architecture" related. The output pluging
> > > can't do right here, this has to be solved at a higher level.
> >
> > That assertion is obviously false... the plugin can resolve this in
> > various ways, if we allow it.
>
> Handling it by breaking replication isn't handling it (e.g. timeouts in
> decoding etc).


IMO, if it's a rare condition and we can abort decoding then recover
cleanly and succeed on retry, that's OK. Not dissimilar to the deadlock
detector. But right now that's not the case, it's possible (however
artificially) to create prepared xacts for which decoding will stall and
not succeed.


> Handling it by rolling back *prepared* transactions
> (which are supposed to be guaranteed to succeed!), isn't either.
>

I agree, we can't rely on anything for which the only way to continue is to
rollback a prepared xact.


> > You can say that in your opinion you prefer to see this handled in
> > some higher level way, though it would be good to hear why and how.
>
> It's pretty obvious why: A bit of DDL by the user shouldn't lead to the
> issues mentioned above.
>

I agree that it shouldn't, and in fact DDL is the main part of why I want
2PC decoding.

What's surprised me is that I haven't actually been able to create any
situations where, with test_decoding, we have such a failure. Not unless I
manually LOCK TABLE pg_attribute, anyway.

Notably, we already disallow prepared xacts that make changes to the
relfilenodemap, which covers a lot of the problem cases like CLUSTERing
system tables.


> > Bottom line here is we shouldn't reject this patch on this point,
>
> I think it definitely has to be rejected because of that.  And I didn't
> bring this up at the last minute, I repeatedly brought it up before.
> Both to Craig and Stas.
>

Yes, and I lost track of it while focusing on the catalog tuple visibility
issues. I warned Stas of this issue when he first mentioned an interest in
decoding of 2PC actually, but haven't kept a proper eye on it since.

Andres and I even discussed this back in the early BDR days, it's not new
and is part of why I poked Stas to try some DDL tests etc. The tests in the
initial patch didn't have enough coverage to trigger any issues - they
didn't actually test decoding of a 2pc xact while it was still in-progress
at all. But even once I added more tests I've actually been unable to
reproduce this in a realistic real world example.

Frankly I'm confused by that, since I would expect an AEL on some_table to
cause decoding of some_table to get stuck. It does not.

That doesn't mean we should accept failure cases and commit something with
holes in it. But it might inform our choices about how we solve those
issues.


> One way to fix this would be to allow decoding to acquire such locks
> (i.e. locks held by the prepared xact we're decoding) - there
> unfortunately are some practical issues with that (e.g. the locking code
> doesnt' necessarily expect a second non-exclusive locker, when there's
> an exclusive one),  or we could add an exception to the locking code to
> simply not acquire such locks.
>

I've been meaning to see if we can use the parallel infrastructure's
session leader infrastructure for this, by making the 2pc fake-proc a
leader and making our decoding session inherit its locks. I haven't dug
into it to see if it's even remotely practical yet, and won't be able to
until early pg11.

We could proceed with the caveat that decoding plugins that use 2pc support
must defer decoding of 2pc xacts containing ddl until commit prepared, or
must take responsibility for ensuring (via a command filter, etc) that
xacts are safe to decode and 2pc lock xacts during decoding. But we're
likely to change the interface for all that when we iterate for pg11 and
I'd rather not carry more BC than we have to. Also, the patch has unsolved
issues with how it keeps track of whether an xact was output at prepare
time or not and suppresses output at commit time.

I'm inclined to shelve the patch for Pg 10. We've only got a couple of days
left, the tests are still pretty minimal. We have open issues around
locking, less than totally satisfactory abort handling, and potential to
skip replay of transactions for both prepare and commit prepared. It's not
ready to go. However, it's definitely to the point where with a little more
work it'll be practical to patch into variants of Pg until we can
mainstream it in Pg 11, which is nice.

--
Craig Ringer

Re: [HACKERS] Supporting huge pages on Windows

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> It's not clear to me what state this patch should be in.  Is there more
> review that needs to be done or is it ready for a committer?

I would most appreciate it if Magnus could review the code, because he gave me 
the most critical comment that the value of huge_page variable should not be 
changed on the fly, and I did so.  I hope that will be the final review.

Regards
Takayuki Tsunakawa



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

Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 29 March 2017 at 08:01, Craig Ringer  wrote:

> I just notice that I failed to remove the docs changes regarding
> dropping slots becoming db-specific, so I'll post a follow-up for that
> in a sec.

Attached.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 5fe01aef643905ec1f6dcffd0f5d583809fc9a21 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 29 Mar 2017 08:03:06 +0800
Subject: [PATCH] Documentation amendments for slot drop on db drop

The "Cleanup slots during drop database" patch incorrectly documented that
dropping logical slots must now be done from the database the slot was created
on. This was the case in an earlier variant of the patch, but not the committed
version.

Also document that idle logical replication slots will be dropped by
DROP DATABASE.
---
 doc/src/sgml/func.sgml  | 3 +--
 doc/src/sgml/protocol.sgml  | 2 --
 doc/src/sgml/ref/drop_database.sgml | 7 +++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 78508d7..ba6f8dd 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18876,8 +18876,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());

 Drops the physical or logical replication slot
 named slot_name. Same as replication protocol
-command DROP_REPLICATION_SLOT. For logical slots, this must
-be called when connected to the same database the slot was created on.
+command DROP_REPLICATION_SLOT.

   
 
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 5f97141..b3a5026 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2034,8 +2034,6 @@ The commands accepted in walsender mode are:
  
   Drops a replication slot, freeing any reserved server-side resources. If
   the slot is currently in use by an active connection, this command fails.
-  If the slot is a logical slot that was created in a database other than
-  the database the walsender is connected to, this command fails.
  
  
   
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index 740aa31..3427139 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -81,6 +81,13 @@ DROP DATABASE [ IF EXISTS ] name
 instead,
which is a wrapper around this command.
   
+
+  
+   Active logical
+   replication slots count as connections and will prevent a
+   database from being dropped. Inactive slots will be automatically
+   dropped when the database is dropped.
+  
  
 
  
-- 
2.5.5


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

Re: [HACKERS] Logical decoding on standby

[Craig Ringer]

On 28 March 2017 at 23:22, Andres Freund  wrote:

>> --- a/doc/src/sgml/protocol.sgml
>> +++ b/doc/src/sgml/protocol.sgml
>> @@ -2034,6 +2034,8 @@ The commands accepted in walsender mode are:
>>   
>>Drops a replication slot, freeing any reserved server-side resources. 
>> If
>>the slot is currently in use by an active connection, this command 
>> fails.
>> +  If the slot is a logical slot that was created in a database other 
>> than
>> +  the database the walsender is connected to, this command fails.
>>   
>>   
>>
>
> Shouldn't the docs in the drop database section about this?

DROP DATABASE doesn't really discuss all the resources it drops, but
I'm happy to add mention of replication slots handling.

I just notice that I failed to remove the docs changes regarding
dropping slots becoming db-specific, so I'll post a follow-up for that
in a sec.

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


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

Re: [HACKERS] logical replication launcher crash on buildfarm

[Petr Jelinek]

On 29/03/17 01:29, Petr Jelinek wrote:
> On 28/03/17 18:05, Petr Jelinek wrote:
>> On 28/03/17 17:55, Robert Haas wrote:
>>> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
>>>  wrote:
 On 28/03/17 04:46, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund  
> wrote:
>>> Btw now that I look at the code, I guess we'll want to get rid of
>>> bgw_main completely in HEAD given that we can't guarantee it will be
>>> valid even for shared_preload_library libraries. For older branches I
>>> would leave things as they are in this regard as there don't seem to be
>>> any immediate issue for standard binaries.
>>
>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>> code in place in < HEAD.
>
> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
> along the lines of
>
> if (bgw_library_name == NULL && bgw_function_name != NULL)
> {
> if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>ParallelQueryMain(blah);
> else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>LogicalReplicationMain(blah);
> }
>
> I think something like that is certainly better for the back-branches,
> because it doesn't cause an ABI break.  But I think it would also be
> fine for master.
>

 I had something slightly more complex like the attached in mind.
>>>
>>> Seems broadly reasonable on a quick look, but I think we should leave
>>> bgw_main intact in 9.6.  It may be working for fine for people who
>>> don't care about Windows, and I'd rather not break it gratuitously.
>>> Can we have two patches, one of which converts the internal stuff to
>>> use the new mechanism and a second of which removes bgw_main?  The
>>> second one, at least, also needs to update the documentation.  (A good
>>> practice when removing an identifier is to run 'git grep
>>> thing_i_am_removing' after removing it...)
>>>
>>
>> Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
>> I just sent what I had before having it all ready mainly because you
>> started talking about code already and I didn't want you to waste time
>> with it needlessly, but it was already 5AM for me ;).
>>
> 
> Okay finally got to it, the 9.6 adds similar mechanism as HEAD (it could
> be simpler there but it seems like same code is better) but does not
> remove the bgw_main.
> 

Sigh, forgot git add for the docs, so one more try...

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


Use-library_name-function_name-for-loading-main-entr-96.patch
Description: binary/octet-stream


Use-library_name-function_name-for-loading-main-entr-HEAD.patch
Description: binary/octet-stream

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

Re: [HACKERS] logical replication launcher crash on buildfarm

[Petr Jelinek]

On 28/03/17 18:05, Petr Jelinek wrote:
> On 28/03/17 17:55, Robert Haas wrote:
>> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
>>  wrote:
>>> On 28/03/17 04:46, Robert Haas wrote:
 On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund  wrote:
>> Btw now that I look at the code, I guess we'll want to get rid of
>> bgw_main completely in HEAD given that we can't guarantee it will be
>> valid even for shared_preload_library libraries. For older branches I
>> would leave things as they are in this regard as there don't seem to be
>> any immediate issue for standard binaries.
>
> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
> code in place in < HEAD.

 I wasn't thinking of introducing bgw_builtin_id.  My idea was just
 along the lines of

 if (bgw_library_name == NULL && bgw_function_name != NULL)
 {
 if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
ParallelQueryMain(blah);
 else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
LogicalReplicationMain(blah);
 }

 I think something like that is certainly better for the back-branches,
 because it doesn't cause an ABI break.  But I think it would also be
 fine for master.

>>>
>>> I had something slightly more complex like the attached in mind.
>>
>> Seems broadly reasonable on a quick look, but I think we should leave
>> bgw_main intact in 9.6.  It may be working for fine for people who
>> don't care about Windows, and I'd rather not break it gratuitously.
>> Can we have two patches, one of which converts the internal stuff to
>> use the new mechanism and a second of which removes bgw_main?  The
>> second one, at least, also needs to update the documentation.  (A good
>> practice when removing an identifier is to run 'git grep
>> thing_i_am_removing' after removing it...)
>>
> 
> Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
> I just sent what I had before having it all ready mainly because you
> started talking about code already and I didn't want you to waste time
> with it needlessly, but it was already 5AM for me ;).
> 

Okay finally got to it, the 9.6 adds similar mechanism as HEAD (it could
be simpler there but it seems like same code is better) but does not
remove the bgw_main.

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


Use-library_name-function_name-for-loading-main-entr-HEAD.patch
Description: binary/octet-stream


Use-library_name-function_name-for-loading-main-entr-96.patch
Description: binary/octet-stream

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

Re: [HACKERS] dsm.c API tweak

[Alvaro Herrera]

Thomas Munro wrote:

> +   if (CurrentResourceOwner)
> +   {
> +   seg->resowner = CurrentResourceOwner;
> +   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
> +   }
> 
> You need to assign seg->resowner = CurrentResourceOwner
> unconditionally here.  Otherwise seg->resowner is uninitialised junk.

Thanks for that.  Pushed, with the comments as I suggested in my other
reply.  If you think they merit more wordsmithing, feel free to suggest
something.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Alvaro Herrera]

I pushed 0002 after some makeup, since it's just cosmetic and not
controversial.  Here's 0003 rebased on top of it.

(Also, I took out the gin and gist changes: it would be wrong to change
that unconditionally, because the 0x pattern appears in indexes that
would be pg_upgraded.  We need a different strategy, if we want to
enable WARM on GiST/GIN indexes.)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From f6ba238dd46416eb29ac43dadac0c69a75f106fe Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 28 Mar 2017 17:39:26 -0300
Subject: [PATCH] Free 3 bits of ItemPointerData.ip_posid

Since we limit block size to 32 kB, the highest offset number used in a
bufpage.h-organized page is 5461, which can be represented with 13 bits.
Therefore, the upper 3 bits in 16-bit ItemPointerData.ip_posid are
unused and this commit reserves them for other purposes.

Author: Pavan Deolasee
---
 src/include/access/htup_details.h |  2 +-
 src/include/storage/itemptr.h | 30 +++---
 src/include/storage/off.h | 11 ++-
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/src/include/access/htup_details.h 
b/src/include/access/htup_details.h
index 7b6285d..d3cc0ad 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -282,7 +282,7 @@ struct HeapTupleHeaderData
  * than MaxOffsetNumber, so that it can be distinguished from a valid
  * offset number in a regular item pointer.
  */
-#define SpecTokenOffsetNumber  0xfffe
+#define SpecTokenOffsetNumber  OffsetNumberPrev(OffsetNumberMask)
 
 /*
  * HeapTupleHeader accessor macros
diff --git a/src/include/storage/itemptr.h b/src/include/storage/itemptr.h
index c21d2ad..74eed4e 100644
--- a/src/include/storage/itemptr.h
+++ b/src/include/storage/itemptr.h
@@ -57,7 +57,7 @@ typedef ItemPointerData *ItemPointer;
  * True iff the disk item pointer is not NULL.
  */
 #define ItemPointerIsValid(pointer) \
-   ((bool) (PointerIsValid(pointer) && ((pointer)->ip_posid != 0)))
+   ((bool) (PointerIsValid(pointer) && (((pointer)->ip_posid & 
OffsetNumberMask) != 0)))
 
 /*
  * ItemPointerGetBlockNumberNoCheck
@@ -84,7 +84,7 @@ typedef ItemPointerData *ItemPointer;
  */
 #define ItemPointerGetOffsetNumberNoCheck(pointer) \
 ( \
-   (pointer)->ip_posid \
+   ((pointer)->ip_posid & OffsetNumberMask) \
 )
 
 /*
@@ -98,6 +98,30 @@ typedef ItemPointerData *ItemPointer;
 )
 
 /*
+ * Get the flags stored in high order bits in the OffsetNumber.
+ */
+#define ItemPointerGetFlags(pointer) \
+( \
+   ((pointer)->ip_posid & ~OffsetNumberMask) >> OffsetNumberBits \
+)
+
+/*
+ * Set the flag bits. We first left-shift since flags are defined starting 0x01
+ */
+#define ItemPointerSetFlags(pointer, flags) \
+( \
+   ((pointer)->ip_posid |= ((flags) << OffsetNumberBits)) \
+)
+
+/*
+ * Clear all flags.
+ */
+#define ItemPointerClearFlags(pointer) \
+( \
+   ((pointer)->ip_posid &= OffsetNumberMask) \
+)
+
+/*
  * ItemPointerSet
  * Sets a disk item pointer to the specified block and offset.
  */
@@ -105,7 +129,7 @@ typedef ItemPointerData *ItemPointer;
 ( \
AssertMacro(PointerIsValid(pointer)), \
BlockIdSet(&((pointer)->ip_blkid), blockNumber), \
-   (pointer)->ip_posid = offNum \
+   (pointer)->ip_posid = (offNum) \
 )
 
 /*
diff --git a/src/include/storage/off.h b/src/include/storage/off.h
index fe8638f..f058fe1 100644
--- a/src/include/storage/off.h
+++ b/src/include/storage/off.h
@@ -26,7 +26,16 @@ typedef uint16 OffsetNumber;
 #define InvalidOffsetNumber((OffsetNumber) 0)
 #define FirstOffsetNumber  ((OffsetNumber) 1)
 #define MaxOffsetNumber((OffsetNumber) (BLCKSZ / 
sizeof(ItemIdData)))
-#define OffsetNumberMask   (0x)/* valid uint16 
bits */
+
+/*
+ * The biggest BLCKSZ we support is 32kB, and each ItemId takes 6 bytes.
+ * That limits the number of line pointers in a page to 32kB/6B = 5461.
+ * Therefore, 13 bits in OffsetNumber are enough to represent all valid
+ * on-disk line pointers.  Hence, we can reserve the high-order bits in
+ * OffsetNumber for other purposes.
+ */
+#define OffsetNumberBits   13
+#define OffsetNumberMask   uint16) 1) << OffsetNumberBits) - 1)
 
 /* 
  * support macros
-- 
2.1.4


-- 
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] Removing binaries

[Magnus Hagander]

On Tue, Mar 28, 2017 at 6:44 PM, Robert Haas  wrote:

> On Tue, Mar 28, 2017 at 12:18 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> On Tue, Mar 28, 2017 at 11:44 AM, Peter Eisentraut
> >>  wrote:
> >>> On 3/21/17 08:12, Robert Haas wrote:
>  I think a big part of the usability problem here comes from the fact
>  that the default database for connections is based on the username,
>  but the default databases that get created have fixed names (postgres,
>  template1).  So the default configuration is one where you can't
>  connect.  Why the heck do we do it that way?
> >
> >>> Historical, probably.  We could ponder changing the way the default
> >>> database is determined, but I don't want to imagine the breakage coming
> >>> out of that.
> >
> >> What do you think would break?
> >
> > Any configuration depending on the existing default?
> >
> > The existing behavior here dates from before we had schemas, so that
> > if users wanted to have private objects they *had* to use separate
> > databases.  Nowadays a schema-per-user within one database makes a lot
> > more sense for many environments, and we even have the default value
> > for search_path set up to make that as painless as possible.  Still,
> > it's not a solution for everybody, particularly not installations
> > that want to keep their users well separated.
> >
> > Perhaps we could satisfy novices by changing the out-of-the-box
> > behavior, but provide some way to select the old behavior for
> > installations that are really depending on it.
>
> Hmm.  I guess that would mean that the same connection string would
> mean something different depending on how you configured this
> behavior, which does not sound like a good idea.  But why not go the
> other way and just create the default database by default, either in
> addition to or instead of the postgres database?  I mean, most people
> probably do this:
>
> initdb
> pg_ctl start
> createdb
>

I would argue that only a very small minority does that. The majority does
one of:

yum install postgresql
setup-postgresql 
systemctl start postgresql

or
apt-get install postgresql
(which auto-initdb's and autostarts)


or
double-clicks graphical installer on windows
(which auto-initdb's and autostarts)


I would bet each one of those have at least one and probably more than one
order of magnitude more users than ever call initdb or pg_ctl manually.


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

Re: [HACKERS] [PATCH] Incremental sort

[Alexander Korotkov]

On Tue, Mar 28, 2017 at 5:27 PM, David Steele  wrote:

> Hi Alexander,
>
> On 3/20/17 10:19 AM, Heikki Linnakangas wrote:
>
>> On 03/20/2017 11:33 AM, Alexander Korotkov wrote:
>>
>>> Please, find rebased patch in the attachment.
>>>
>>
>> I had a quick look at this.
>>
>
> <...>
>
> According to 'perf', 85% of the CPU time is spent in ExecCopySlot(). To
>> alleviate that, it might be worthwhile to add a special case for when
>> the group contains exactly one group, and not put the tuple to the
>> tuplesort in that case. Or if we cannot ensure that the Incremental Sort
>> is actually faster, the cost model should probably be smarter, to avoid
>> picking an incremental sort when it's not a win.
>>
>
> This thread has been idle for over a week.  Please respond with a new
> patch by 2017-03-30 00:00 AoE (UTC-12) or this submission will be marked
> "Returned with Feedback".


Thank you for reminder!

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 1:17 PM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> I don't see anything wrong with adding roles in pg_authid.h with a #define'd
>> Oid.  That's actually pretty helpful for anyone writing code against the 
>> database,
>> as they don't have to look up the Oid of the role.
> 
>> But why not then grant privileges to that role in information_schema.sql? 
> 
> To the extent that the desired privileges can be represented at the SQL
> level, I agree that that's a better solution than hard-wiring checks in C
> code.  The problem comes in with cases where that's not fine-grained
> enough.  Consider a policy like "anybody can select from pg_stat_activity,
> but unless you have privilege X, the query column should read as nulls
> except for sessions belonging to you".  That behavior can't realistically
> be represented as a separate SQL privilege.  Right now we have "privilege
> X" for this purpose hard-coded as "is superuser", but it would be much
> better if it were associated with a grantable role.

Many thanks for all the explanation.  I now understand better what the
patch is trying to do, and have (with some experimentation) seen flaws
in what I was saying upthread.  I find the notion of a role not being a
group of privileges but instead actually being a privilege confusing, and
it made it hard to think about the security implications of the proposal.
I'm accustomed to the idea of being able to revoke a privilege from a
role, and that doesn't work if the "role" is in some sense a privilege, not
a role.  I might find it all easier to think about if we named these things
privileges and not roles, like pg_read_stats_privilege instead of
pg_read_stats_role, and then we could grant pg_read_stats_privilege
to roles.  But I recall you were saying upthread that you did not want to
have privilege bits for each of these types of things.  I hope this feature
is worth the confusion it causes.

mark

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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Andres Freund]

Hi,

On 2017-03-27 22:33:03 -0700, Andres Freund wrote:
> On 2017-03-23 20:35:09 +1300, Thomas Munro wrote:
> > Here is a new patch series responding to feedback from Peter and Andres:
> 
> Here's a review of 0007 & 0010 together - they're going to have to be
> applied together anyway...
> ...
> ok, ENOTIME for today...

Continuing, where I dropped of tiredly yesterday.


-   ExecHashJoinSaveTuple(tuple,
- hashvalue,
- 
>innerBatchFile[batchno]);
+   if (HashJoinTableIsShared(hashtable))
+   sts_puttuple(hashtable->shared_inner_batches, batchno, 
,
+tuple);
+   else
+   ExecHashJoinSaveTuple(tuple,
+ hashvalue,
+ 
>innerBatchFile[batchno]);
}
 }

Why isn't this done inside of ExecHashJoinSaveTuple?




@@ -1280,6 +1785,68 @@ ExecHashTableReset(HashJoinTable hashtable)

+   /* Rewind the shared read heads for this batch, inner 
and outer. */
+   
sts_prepare_parallel_read(hashtable->shared_inner_batches,
+ 
curbatch);
+   
sts_prepare_parallel_read(hashtable->shared_outer_batches,
+ 
curbatch);

It feels somewhat wrong to do this in here, rather than on the callsites.

+   }
+
+   /*
+* Each participant needs to make sure that data it has written 
for
+* this partition is now read-only and visible to other 
participants.
+*/
+   sts_end_write(hashtable->shared_inner_batches, curbatch);
+   sts_end_write(hashtable->shared_outer_batches, curbatch);
+
+   /*
+* Wait again, so that all workers see the new hash table and 
can
+* safely read from batch files from any participant because 
they have
+* all ended writing.
+*/
+   Assert(BarrierPhase(>shared->barrier) ==
+  PHJ_PHASE_RESETTING_BATCH(curbatch));
+   BarrierWait(>shared->barrier, 
WAIT_EVENT_HASH_RESETTING);
+   Assert(BarrierPhase(>shared->barrier) ==
+  PHJ_PHASE_LOADING_BATCH(curbatch));
+   ExecHashUpdate(hashtable);
+
+   /* Forget the current chunks. */
+   hashtable->current_chunk = NULL;
+   return;
+   }
 
/*
 * Release all the hash buckets and tuples acquired in the prior pass, 
and
@@ -1289,10 +1856,10 @@ ExecHashTableReset(HashJoinTable hashtable)
oldcxt = MemoryContextSwitchTo(hashtable->batchCxt);
 
/* Reallocate and reinitialize the hash bucket headers. */
-   hashtable->buckets = (HashJoinTuple *)
-   palloc0(nbuckets * sizeof(HashJoinTuple));
+   hashtable->buckets = (HashJoinBucketHead *)
+   palloc0(nbuckets * sizeof(HashJoinBucketHead));
 
-   hashtable->spaceUsed = nbuckets * sizeof(HashJoinTuple);
+   hashtable->spaceUsed = nbuckets * sizeof(HashJoinBucketHead);
 
/* Cannot be more than our previous peak; we had this size before. */
Assert(hashtable->spaceUsed <= hashtable->spacePeak);
@@ -1301,6 +1868,22 @@ ExecHashTableReset(HashJoinTable hashtable)
 
/* Forget the chunks (the memory was freed by the context reset above). 
*/
hashtable->chunks = NULL;
+
+   /* Rewind the shared read heads for this batch, inner and outer. */
+   if (hashtable->innerBatchFile[curbatch] != NULL)
+   {
+   if (BufFileSeek(hashtable->innerBatchFile[curbatch], 0, 0L, 
SEEK_SET))
+   ereport(ERROR,
+   (errcode_for_file_access(),
+  errmsg("could not rewind hash-join temporary 
file: %m")));
+   }
+   if (hashtable->outerBatchFile[curbatch] != NULL)
+   {
+   if (BufFileSeek(hashtable->outerBatchFile[curbatch], 0, 0L, 
SEEK_SET))
+   ereport(ERROR,
+   (errcode_for_file_access(),
+  errmsg("could not rewind hash-join temporary 
file: %m")));
+   }
 }
 
 /*
@@ -1310,12 +1893,21 @@ ExecHashTableReset(HashJoinTable hashtable)
 void
 ExecHashTableResetMatchFlags(HashJoinTable hashtable)
 {
+   dsa_pointer chunk_shared = InvalidDsaPointer;
HashMemoryChunk chunk;
HashJoinTuple tuple;
int i;
 
/* Reset all flags in the main table ... */
-   chunk = hashtable->chunks;
+   if 

Re: [HACKERS] Monitoring roles patch

[Tom Lane]

Mark Dilger  writes:
> I don't see anything wrong with adding roles in pg_authid.h with a #define'd
> Oid.  That's actually pretty helpful for anyone writing code against the 
> database,
> as they don't have to look up the Oid of the role.

> But why not then grant privileges to that role in information_schema.sql? 

To the extent that the desired privileges can be represented at the SQL
level, I agree that that's a better solution than hard-wiring checks in C
code.  The problem comes in with cases where that's not fine-grained
enough.  Consider a policy like "anybody can select from pg_stat_activity,
but unless you have privilege X, the query column should read as nulls
except for sessions belonging to you".  That behavior can't realistically
be represented as a separate SQL privilege.  Right now we have "privilege
X" for this purpose hard-coded as "is superuser", but it would be much
better if it were associated with a grantable role.

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] [COMMITTERS] pgsql: Improve performance of find_all_inheritors()

[Aleksander Alekseev]

Hi, Robert.

> I'm a little worried that this will be noticeably slower when the
> number of partitions is small, like 4 or 8 or 16.  In other places,
> we've found it necessary to support both a list-based strategy and a
> hash-based strategy to avoid regressing small cases (e.g.
> join_rel_list + join_rel_hash in PlannerInfo, ResourceArray in
> resowner.c, etc).

I've checked it [1]. Apparently in this particular case patches make code
faster on small amount of partitions as well. 

[1] https://postgr.es/m/20170324132258.GB16830%40e733.localdomain

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Monitoring roles patch

[Dave Page]

Hi

On Tue, Mar 28, 2017 at 2:22 PM, Stephen Frost  wrote:
> Dave,
>
> * Dave Page (dp...@pgadmin.org) wrote:
>> OK, so before I start hacking again, here's a proposal based on my
>> understanding of folks comments, and so open questions. If I can get
>> agreement and answers, I'll be able to break out vi again without
>> (hopefully) too many more revisions:
>>
>> pg_read_all_stats: Will have C-coded access to pg_stats views and
>> pg_*_size that are currently hard-coded to superuser
>
> Not quite sure what you mean here by 'C-coded access to pg_stats
> *views*', but I'm guessing that isn't exactly what you meant since I'm
> sure we aren't going to change how view permissions are done in this
> patch.  I take it you mean access to the functions under the views?  If
> so, I believe this is correct.

Right, sorry I wasn't clear.

>> pg_read_all_settings: Will have C-coded access to read GUCs that are
>> currently hard-coded to the superuser
>
> Right.
>
>> pg_monitor: Will include pg_read_all_stats and pg_read_all_settings,
>> and all explicitly GRANTable rights, e.g. in contrib modules.
>
> Right.
>
>> Patch to be rebased on Simon's updated version.
>
> Right.
>
>> Questions:
>>
>> - pg_stat_statements has a hard-coded superuser check. Shall I remove
>> that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor?
>
> pg_stat_statements shouldn't have ever had that superuser check and it
> shouldn't have ever used '==' for the user check, it should have been
> using 'has_privs_of_role()' from the start, which means that the
> superuser check isn't necessary.
>
> I don't think we should remove that check, nor should we REVOKE EXECUTE
> from public for the function.  We *should* add a hard-coded role check
> to allow another role which isn't a member of the role whose query it is
> to view the query.  That shouldn't be pg_monitor, of course (as
> discussed).  I don't think pg_read_all_stats or pg_read_all_settings
> really covers this case either- this is more like pg_read_all_queries
> and should also be used for pg_stat_activity.

OK, so essentially what I did, except s/pg_read_all_stats/pg_read_all_queries ?

> That would then be granted to pg_monitor.
>
>> - pgrowlocks has hard-coded access to superuser and users with SELECT
>> on the table being examined. How should this be handled?
>
> I don't see any hard-coded superuser check?  There is a
> pg_class_aclcheck() for SELECT rights on the table.  I like the idea of
> having another way to grant access to run this function on a table
> besides giving SELECT rights on the entire table to the user.  This
> would fall under the mandate of the role described in your next bullet,
> in my view.
>
>> - Stephen suggested a separate role for functions that can lock
>> tables. Is this still desired, or shall we just grant access to
>> pg_monitor (I think the latter is fine)?
>
> Right, I was thinking something like pg_stat_all_tables or
> pg_stat_scan_tables or similar.  We would add that (perhaps also with a
> SELECT check like pgrowlocks has) for the other functions like
> pgstattuple and pg_freespacemap and pg_visibility.

So pgstattuple, pg_sfreespacemap, pg_visibility and pgrowlocks to be
allowed access from members of pg_stat_scan_tables, which in turn is
granted to pg_monitor?

Thanks!

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 11:52 AM, Tom Lane  wrote:
> 
> Mark Dilger  writes:
>> After a bit of introspection, I think what is really bothering me is not the
>> inability to revoke permissions, since as you say I can choose to not assign
>> the role to anybody.  What bothers me is that this feature implicitly 
>> redefines
>> what is meant by the keyword PUBLIC.  If I go around the database
>> revoking privileges on everything from PUBLIC, I should end up with
>> a database that is inaccessible to anyone but superuser, right?
> 
> Ummm ... not if you've granted specific permissions to some users.
> If you did "GRANT SELECT ON TABLE x TO joe", no amount of public-privilege
> revocation makes that go away.  This isn't so much different.

I think it is pretty different.  It is not unreasonable to lock down a freshly 
installed
database by revoking privileges on various objects from PUBLIC prior to creating
any users.  Before the proposed change, that would have different consequences
than after the proposed change.

> It's fair to object that the problem with this approach is that the
> permissions available to these special roles aren't visible in the
> privilege-related system catalog columns.  But we've been around on that
> discussion and agreed that it's impractical to have a separate GRANT bit
> for every little bit of privilege that someone might want.  So the plan is
> to have these special roles that are known at the C-code level, and then
> granting one of these roles to user X effectively grants user X certain
> fine-grained privileges.  

I'm still struggling to understand the motivation for hardcoded permissions
checks.

I don't see anything wrong with adding roles in pg_authid.h with a #define'd
Oid.  That's actually pretty helpful for anyone writing code against the 
database,
as they don't have to look up the Oid of the role.

But why not then grant privileges to that role in information_schema.sql? 
That would avoid all the special case logic in the code, and would mean that
people could add and remove privileges from the role to suit their own
security policies.  For somebody configuring security policies on a database
prior to creating any users, these changes could be made at that time.

No need to write up an explanation.  I'm guessing you have a link to a
discussion where this was all discussed to death.

> But you can't see anything except the role grant
> in the catalogs.  You just have to read the documentation to find out what
> that really means.
> 
>> But after this proposed
>> change, IIUC, there would still be a bit of access available to this/these
>> proposed non-superuser role(s) which have hardcoded permissions.
> 
> Right, but they are roles not users, ie they do not have the LOGIN bit.
> So the only way to use them is via GRANT.
> 
> I think there is a fair question about how to document this clearly, but
> the design seems reasonable.
> 
>   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] Protection lost in expression eval changeover

[Andres Freund]

On 2017-03-28 15:24:28 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund  writes:
> >> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
> >>> I don't see a strong reason why we need to allow a dropped column to go
> >>> to null while we throw an immediate error for a change in column type.
> >>> (If there is some reason, hopefully beta testing will find it.)
> 
> >> Ok.  You're doing that?
> 
> > Yeah, I'll go do that.
> 
> Or maybe not: turns out we have regression test cases that depend on this
> behavior, cf the bit in create_view.sql about
> 
> -- this perhaps should be rejected, but it isn't:
> alter table tt14t drop column f3;
> -- f3 is still in the view but will read as nulls
> 
> You'd proposed changing that, which I agree with in principle, but
> I thought your patch wasn't right.  Maybe we need to work harder on
> that.
> 
> (I'm not actually sure right at the moment why this case isn't failing
> in HEAD.  Maybe plpgsql is replacing the dropped column with nulls in
> its result rows, so that whether the outer query special-cases them or
> not doesn't affect the visible output.)
> 
> Or we could just throw an error anyway.  I'm not sure there's any
> strong reason to allow such cases to work.  I think the regression
> tests were only put there to ensure they don't crash, not to memorialize
> behavior we consider essential.

Yea, cases like that was what I was referring to with changing behaviour
- I think it's ok to error out, as long as we do so reliably.

- Andres


-- 
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] Protection lost in expression eval changeover

[Tom Lane]

I wrote:
> Andres Freund  writes:
>> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
>>> I don't see a strong reason why we need to allow a dropped column to go
>>> to null while we throw an immediate error for a change in column type.
>>> (If there is some reason, hopefully beta testing will find it.)

>> Ok.  You're doing that?

> Yeah, I'll go do that.

Or maybe not: turns out we have regression test cases that depend on this
behavior, cf the bit in create_view.sql about

-- this perhaps should be rejected, but it isn't:
alter table tt14t drop column f3;
-- f3 is still in the view but will read as nulls

You'd proposed changing that, which I agree with in principle, but
I thought your patch wasn't right.  Maybe we need to work harder on
that.

(I'm not actually sure right at the moment why this case isn't failing
in HEAD.  Maybe plpgsql is replacing the dropped column with nulls in
its result rows, so that whether the outer query special-cases them or
not doesn't affect the visible output.)

Or we could just throw an error anyway.  I'm not sure there's any
strong reason to allow such cases to work.  I think the regression
tests were only put there to ensure they don't crash, not to memorialize
behavior we consider essential.

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] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

2017-03-28 20:29 GMT+02:00 Petr Jelinek :

> On 28/03/17 19:43, Pavel Stehule wrote:
> > Hi
> >
> > rebased due last changes in pg_exec.c
> >
>
> Thanks, I went over this and worked over the documentation/comments a
> bit (attached updated version of the patch with my changes).
>
> From my side this can go to committer.
>

Thank you

Pavel


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

Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

[Nikolay Shaplov]

В письме от 26 марта 2017 15:02:12 пользователь Alvaro Herrera написал:
> Nikolay Shaplov wrote:
> > If I would think about it now: we always know how many options we will
> > have. So we can just pass this number to palloc and assert if somebody
> > adds more options then expected... What do yo think about it.
> 
> I think we need to preserve the ability to add custom options, because
> extensions may want to do that.

I've been thinking about it for a while... I think this might lead to reducing 
the quality of the code...

For example: There was 10 options for some relation type. I decided to add one 
more option, but i did not ++ the number of options for 
allocateOptionsCatalog. So space for 10 options were allocated, and then when 
11th option is added, optionCatalogAddItem would allocate space for ten more 
options, and nine of them would not be used. And nobody will notice it.

So, I see here four solutions:

0. Leave it as it was. (We both do not like it)
1. Forbid dynamic number of options (you do not like it)
2. Allow dynamic number of options only for special cases, and in all other 
cases make them strict, and asserting if option number is wrong. This can be 
done in two ways:
2a. Add strict boolean flag, that tells if allow to add more options or not
2b. Do not add any flags, but if number of items is specified, then process 
number of items in strict mode. If it is set to -1, then do as it is done now, 
totally dynamically.

I would prefer 2b, if you sure that somebody will need dynamic number of 
options.

PS. I hardly believe that there can be dynamic number of options, as this 
options later are mapped into C-structure that is quite static. No use case 
comes into my mind, where I would like to have dynamic number of options, not 
knowing at build time, how many of them there would be.



-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.


-- 
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] reorder tablespaces in basebackup tar stream for backup_label

[Fujii Masao]

On Tue, Feb 21, 2017 at 7:17 PM, Michael Banck
 wrote:
> Hi,
>
> currently, the backup_label and (I think) the tablespace_map files are
> (by design) conveniently located at the beginning of the main tablespace
> tarball when making a basebackup. However, (by accident or also by
> design?) the main tablespace is the last tarball[1] to be streamed via
> the BASE_BACKUP replication protocol command.
>
> For pg_basebackup, this is not a real problem, as either each tablespace
> is its own tarfile (in tar format mode), or the files are extracted
> anyway (in plain mode).
>
> However, third party tools using the BASE_BACKUP command might want to
> extract the backup_label, e.g. in order to figure out the START WAL
> LOCATION.

The first result set that BASE BACKUP sends back to a client contains
the START WAL LOCATION. So at first, ISTM that you don't need backup_label
for that purpose.

If your need other information except START WAL LOCATION at the beginning of
base backup and they are very useful for many third-party softwares,
you can add them into that first result set. If you do this, you can
retrieve them
at the beginning even when WAL files are included in the backup.

Regards,

-- 
Fujii Masao


-- 
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] Protection lost in expression eval changeover

[Tom Lane]

Andres Freund  writes:
> On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
>> I don't see a strong reason why we need to allow a dropped column to go
>> to null while we throw an immediate error for a change in column type.
>> (If there is some reason, hopefully beta testing will find it.)

> Ok.  You're doing that?

Yeah, I'll go do 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] Monitoring roles patch

[Tom Lane]

Mark Dilger  writes:
> After a bit of introspection, I think what is really bothering me is not the
> inability to revoke permissions, since as you say I can choose to not assign
> the role to anybody.  What bothers me is that this feature implicitly 
> redefines
> what is meant by the keyword PUBLIC.  If I go around the database
> revoking privileges on everything from PUBLIC, I should end up with
> a database that is inaccessible to anyone but superuser, right?

Ummm ... not if you've granted specific permissions to some users.
If you did "GRANT SELECT ON TABLE x TO joe", no amount of public-privilege
revocation makes that go away.  This isn't so much different.

It's fair to object that the problem with this approach is that the
permissions available to these special roles aren't visible in the
privilege-related system catalog columns.  But we've been around on that
discussion and agreed that it's impractical to have a separate GRANT bit
for every little bit of privilege that someone might want.  So the plan is
to have these special roles that are known at the C-code level, and then
granting one of these roles to user X effectively grants user X certain
fine-grained privileges.  But you can't see anything except the role grant
in the catalogs.  You just have to read the documentation to find out what
that really means.

> But after this proposed
> change, IIUC, there would still be a bit of access available to this/these
> proposed non-superuser role(s) which have hardcoded permissions.

Right, but they are roles not users, ie they do not have the LOGIN bit.
So the only way to use them is via GRANT.

I think there is a fair question about how to document this clearly, but
the design seems reasonable.

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] Protection lost in expression eval changeover

[Andres Freund]

On 2017-03-28 14:43:38 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
> >> So it seems like we are missing some needed protection.  I'm inclined
> >> to think that it'd be all right to just throw an error immediately in
> >> CheckVarSlotCompatibility if the target column is dropped.
> 
> > Hm - so far we've pretty widely only set columns to NULL in that
> > case. You don't see concerns with triggering errors in cases we
> > previously hadn't?
> 
> Well, in principle these errors ought to be unreachable at all; they're
> only there to backstop any possible failure to notice stale plans.

Not entirely - e.g. I think some of that's reachable when composite
types are involved.  But it's probably ok to error out anyway.


> I don't see a strong reason why we need to allow a dropped column to go
> to null while we throw an immediate error for a change in column type.
> (If there is some reason, hopefully beta testing will find it.)

Ok.  You're doing that?

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] Monitoring roles patch

[Stephen Frost]

Greetings,

* Mark Dilger (hornschnor...@gmail.com) wrote:
> After a bit of introspection, I think what is really bothering me is not the
> inability to revoke permissions, since as you say I can choose to not assign
> the role to anybody.  What bothers me is that this feature implicitly 
> redefines
> what is meant by the keyword PUBLIC.  If I go around the database
> revoking privileges on everything from PUBLIC, I should end up with
> a database that is inaccessible to anyone but superuser, right?  

While I accept your concern and confusion, this isn't accurate.
REVOKE'ing rights from PUBLIC only removes those rights GRANT'd to
PUBLIC, it doesn't remove rights which have been GRANT'd to other users.

In a degenerate system where the only roles which exist are the
superuser role and the pseudo-role 'public', you would be correct with
your statement above, but this patch doesn't change that because you
will not have GRANT'd the new role to anyone.

> All views,
> functions, tables, etc., would all be locked down.  But after this proposed
> change, IIUC, there would still be a bit of access available to this/these
> proposed non-superuser role(s) which have hardcoded permissions.

These non-superuser roles can't be logged into and if they are not
GRANT'd to anyone then it's essentially the same as if they don't exist.

> That's quite a significant change to the security model of the database,
> and I don't think it is something users would expect from the release notes
> if the release notes for this feature say something like:
> 
>   "added database monitoring functionality"

Having more in the release notes and in the documentation is certainly
important, but this is not changing the security model of the database
in any significant way.

> To be fair, I have not tried to revoke everything from everybody on a
> database to verify that their aren't already problems of this sort.  Perhaps
> the cows have already left the barn.

Right, which means that, in addition to the points made above, this
isn't a use-case which is actually even all that interesting to
consider.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Protection lost in expression eval changeover

[Tom Lane]

Andres Freund  writes:
> On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
>> So it seems like we are missing some needed protection.  I'm inclined
>> to think that it'd be all right to just throw an error immediately in
>> CheckVarSlotCompatibility if the target column is dropped.

> Hm - so far we've pretty widely only set columns to NULL in that
> case. You don't see concerns with triggering errors in cases we
> previously hadn't?

Well, in principle these errors ought to be unreachable at all; they're
only there to backstop any possible failure to notice stale plans.
I don't see a strong reason why we need to allow a dropped column to go
to null while we throw an immediate error for a change in column type.
(If there is some reason, hopefully beta testing will find it.)

> I wonder if it'd not be better to add a branch to slot_deform_tuple's
> main loop like

Much rather not do 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] Getting server crash after running sqlsmith

[Tom Lane]

tushar  writes:
> After runinng sqlsmith against latest sources of PG v10  , able to see a 
> crash -

Hm ... I don't see a crash here, but I wonder whether you have parameters
set that would cause this query to be run as a parallel query?  Because
pg_rotate_logfile() is marked as parallel-safe in pg_proc, which seems
probably insane.

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] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 11:06 AM, Mark Dilger  wrote:
> 
> 
>> On Mar 28, 2017, at 9:47 AM, Dave Page  wrote:
>> 
>>> Does
>>> pg_read_all_stats still have access to stats for mysecuretable?
>> 
>> Yes, because the ACL on the table controls reading/writing the data in
>> the table. It doesn't have any bearing on any kind of table metadata.
>> A user who has no privileges on a table can already look at (for
>> example) pg_stat_all_tables and see the sort of info you're talking
>> about. This patch would just allow members of a specific role get the
>> on-disk size as well, *if* you choose to use it.
> 
> I don't entirely agree here.  Security conscious database users may well
> restrict access to that view.  I added a check to the regression tests to
> verify that it works:
> 
> + SET ROLE regress_locktable_user;
> + SELECT COUNT(*) FROM pg_stat_all_tables;
> +  count 
> + ---
> +290
> + (1 row)
> + 
> + RESET ROLE;
> + REVOKE ALL PRIVILEGES ON pg_stat_all_tables FROM PUBLIC;
> + SET ROLE regress_locktable_user;
> + SELECT COUNT(*) FROM pg_stat_all_tables;
> + ERROR:  permission denied for relation pg_stat_all_tables
> + RESET ROLE;
> + SELECT COUNT(*) FROM pg_stat_all_tables;
> +  count 
> + ---
> +292
> + (1 row)
> + 
> 
> The inability to revoke access to this sort of information being proposed
> makes me a bit uneasy.  Mostly, I think, I'm bothered because there may
> be people who have revoked privileges on a lot of things, thereby restricting
> access to superuser, who won't necessarily notice this new feature in
> postgres 10.  That could be a source of security holes that we get blamed
> for.

After a bit of introspection, I think what is really bothering me is not the
inability to revoke permissions, since as you say I can choose to not assign
the role to anybody.  What bothers me is that this feature implicitly redefines
what is meant by the keyword PUBLIC.  If I go around the database
revoking privileges on everything from PUBLIC, I should end up with
a database that is inaccessible to anyone but superuser, right?  All views,
functions, tables, etc., would all be locked down.  But after this proposed
change, IIUC, there would still be a bit of access available to this/these
proposed non-superuser role(s) which have hardcoded permissions.

That's quite a significant change to the security model of the database,
and I don't think it is something users would expect from the release notes
if the release notes for this feature say something like:

"added database monitoring functionality"

To be fair, I have not tried to revoke everything from everybody on a
database to verify that their aren't already problems of this sort.  Perhaps
the cows have already left the barn.

mark



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

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Petr Jelinek]

On 28/03/17 19:43, Pavel Stehule wrote:
> Hi
> 
> rebased due last changes in pg_exec.c
> 

Thanks, I went over this and worked over the documentation/comments a
bit (attached updated version of the patch with my changes).

>From my side this can go to committer.

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


Add-plan_cache-control-PRAGMA-to-pl-pgsql.patch
Description: binary/octet-stream

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

Re: [HACKERS] Monitoring roles patch

[Stephen Frost]

Greetings,

* Mark Dilger (hornschnor...@gmail.com) wrote:
> The inability to revoke access to this sort of information being proposed
> makes me a bit uneasy.  

What data are you concerned about, specifically?

> Mostly, I think, I'm bothered because there may
> be people who have revoked privileges on a lot of things, thereby restricting
> access to superuser, who won't necessarily notice this new feature in
> postgres 10.  That could be a source of security holes that we get blamed
> for.

There is no access granted by adding this role without an admin granting
access to this role to some other user.  If they make incorrect
assumptions about what granting access to this role means then I'm
afraid that's their issue, not ours.

> Please note that I'm not specifically opposed to this work, and see a lot
> of merit here.  I'm just thinking about unintended consequences.

Certainly, good to think of, but I don't believe there's a concern here.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Monitoring roles patch

[Stephen Frost]

Dave,

* Dave Page (dp...@pgadmin.org) wrote:
> OK, so before I start hacking again, here's a proposal based on my
> understanding of folks comments, and so open questions. If I can get
> agreement and answers, I'll be able to break out vi again without
> (hopefully) too many more revisions:
> 
> pg_read_all_stats: Will have C-coded access to pg_stats views and
> pg_*_size that are currently hard-coded to superuser

Not quite sure what you mean here by 'C-coded access to pg_stats
*views*', but I'm guessing that isn't exactly what you meant since I'm
sure we aren't going to change how view permissions are done in this
patch.  I take it you mean access to the functions under the views?  If
so, I believe this is correct.

> pg_read_all_settings: Will have C-coded access to read GUCs that are
> currently hard-coded to the superuser

Right.

> pg_monitor: Will include pg_read_all_stats and pg_read_all_settings,
> and all explicitly GRANTable rights, e.g. in contrib modules.

Right.

> Patch to be rebased on Simon's updated version.

Right.

> Questions:
> 
> - pg_stat_statements has a hard-coded superuser check. Shall I remove
> that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor?

pg_stat_statements shouldn't have ever had that superuser check and it
shouldn't have ever used '==' for the user check, it should have been
using 'has_privs_of_role()' from the start, which means that the
superuser check isn't necessary.

I don't think we should remove that check, nor should we REVOKE EXECUTE
from public for the function.  We *should* add a hard-coded role check
to allow another role which isn't a member of the role whose query it is
to view the query.  That shouldn't be pg_monitor, of course (as
discussed).  I don't think pg_read_all_stats or pg_read_all_settings
really covers this case either- this is more like pg_read_all_queries
and should also be used for pg_stat_activity.

That would then be granted to pg_monitor.

> - pgrowlocks has hard-coded access to superuser and users with SELECT
> on the table being examined. How should this be handled?

I don't see any hard-coded superuser check?  There is a
pg_class_aclcheck() for SELECT rights on the table.  I like the idea of
having another way to grant access to run this function on a table
besides giving SELECT rights on the entire table to the user.  This
would fall under the mandate of the role described in your next bullet,
in my view.

> - Stephen suggested a separate role for functions that can lock
> tables. Is this still desired, or shall we just grant access to
> pg_monitor (I think the latter is fine)?

Right, I was thinking something like pg_stat_all_tables or
pg_stat_scan_tables or similar.  We would add that (perhaps also with a
SELECT check like pgrowlocks has) for the other functions like
pgstattuple and pg_freespacemap and pg_visibility.

> - Based on Peter's concerns, is pg_read_all_stats the right name?
> Maybe pg_read_monitoring_stats?

I'd rather not get too wrapped up in the naming..  The documentation is
the important part here, imv.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Missing increment of vacrelstats->pinskipped_pages

[Jim Nasby]

lazy_vacuum_heap() does not count pages that it skips due to not 
obtaining the buffer cleanup lock. vacuum_pinskipped.patch fixes that. 
That should be backpatched to 9.5.


vacuum_comment.patch cleans up a comment in lazy_scan_heap().
--
Jim C. Nasby, Data Architect   j...@nasby.net
512.569.9461 (cell) http://jim.nasby.net
diff --git a/src/backend/commands/vacuumlazy.c 
b/src/backend/commands/vacuumlazy.c
index 5b43a66bdc..6f7a5b4818 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -530,9 +530,9 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats 
*vacrelstats,
 * safely set for relfrozenxid or relminmxid.
 *
 * Before entering the main loop, establish the invariant that
-* next_unskippable_block is the next block number >= blkno that's not 
we
-* can't skip based on the visibility map, either all-visible for a
-* regular scan or all-frozen for an aggressive scan.  We set it to
+* next_unskippable_block is the next block number >= blkno that we
+* can't skip based on the visibility map (either all-visible for a
+* regular scan or all-frozen for an aggressive scan).  We set it to
 * nblocks if there's no such block.  We also set up the skipping_blocks
 * flag correctly at this stage.
 *
diff --git a/src/backend/commands/vacuumlazy.c 
b/src/backend/commands/vacuumlazy.c
index 5b43a66bdc..5a5a4ba48b 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1404,6 +1404,7 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
if (!ConditionalLockBufferForCleanup(buf))
{
ReleaseBuffer(buf);
+   vacrelstats->pinskipped_pages++;
++tupindex;
continue;
}

-- 
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] Protection lost in expression eval changeover

[Andres Freund]

Hi,

On 2017-03-28 13:52:50 -0400, Tom Lane wrote:
> CheckVarSlotCompatibility contains the comment
> 
>* Note: we allow a reference to a dropped attribute.  slot_getattr will
>* force a NULL result in such cases.
> 
> While still true, that second sentence is now quite irrelevant, because we
> don't go through slot_getattr anymore.

Note it was already quite irrelevant before :( - if slot_getattr(n) was
called after slot_getattr(n+x), it'd go to the fastpath exit, and thus
not check anything.  Same


> So it seems like we are missing some needed protection.  I'm inclined
> to think that it'd be all right to just throw an error immediately in
> CheckVarSlotCompatibility if the target column is dropped.

Hm - so far we've pretty widely only set columns to NULL in that
case. You don't see concerns with triggering errors in cases we
previously hadn't?


I wonder if it'd not be better to add a branch to slot_deform_tuple's
main loop like

/*
 * If the attribute has been dropped, set to NULL. That's important
 * because various codepaths assume that the first slot->tts_nvalid
 * attributes can be accessed directly via tts_values/isnull.
 */
if (unlikely(thisatt->attisdropped))
{
values[attnum] = (Datum) 0;
isnull[attnum] = true;
}

It's annoying to add a branch there, it's a pretty hot function, but it
seems like quite a worthwhile safety measure.


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] pg_stat_wal_write statistics view

[Fujii Masao]

On Tue, Mar 28, 2017 at 1:40 PM, Haribabu Kommi
 wrote:
>
>
> On Mon, Mar 27, 2017 at 1:27 PM, Haribabu Kommi 
> wrote:
>>
>>
>>
>> On Sat, Mar 25, 2017 at 6:40 AM, Fujii Masao 
>> wrote:
>>>
>>> On Wed, Feb 15, 2017 at 12:53 PM, Haribabu Kommi
>>>  wrote:
>>> >
>>> >
>>> > On Wed, Feb 8, 2017 at 9:36 PM, Amit Kapila 
>>> > wrote:
>>> >>
>>> >> On Tue, Feb 7, 2017 at 11:47 AM, Haribabu Kommi
>>> >>  wrote:
>>> >> > Hi Hackers,
>>> >> >
>>> >> > I just want to discuss adding of a new statistics view that provides
>>> >> > the information of wal writing details as follows
>>> >> >
>>> >>
>>> >> +1.  I think it will be useful to observe WAL activity.
>>> >
>>> >
>>> > Thanks for your opinion.
>>> >
>>> >> > postgres=# \d pg_stat_wal_writer
>>> >> > View "pg_catalog.pg_stat_wal_writer"
>>> >> > Column |   Type   | Collation |
>>> >> > Nullable
>>> >> > |
>>> >> > Default
>>> >> >
>>> >> >
>>> >> > ---+--+---+--+-
>>> >> >  num_backend_writes   | bigint   |   |
>>> >> > |
>>> >> >  num_total_writes  | bigint   |   |
>>> >> > |
>>> >> >  num_blocks  | bigint   |   |  |
>>> >> >  total_write_time   | bigint|   |  |
>>> >> >  stats_reset   | timestamp with time zone |   |
>>> >> > |
>>> >> >
>>> >> > The columns of the view are
>>> >> > 1. Total number of xlog writes that are called from the backend.
>>> >> > 2. Total number of xlog writes that are called from both backend
>>> >> >  and background workers. (This column can be changed to just
>>> >> >  display on the background writes).
>>> >> > 3. The number of the blocks that are written.
>>> >> > 4. Total write_time of the IO operation it took, this variable data
>>> >> > is
>>> >> > filled only when the track_io_timing GUC is enabled.
>>> >>
>>> >> So, here is *write_time* the total time system has spent in WAL
>>> >> writing before the last reset?
>>> >
>>> >
>>> > total write_time spent in WAL writing "after" the last reset in
>>> > milliseconds.
>>> >
>>> >> I think there should be a separate column for write and sync time.
>>> >>
>>> >
>>> > Added.
>>> >
>>> >>
>>> >> > Or it is possible to integrate the new columns into the existing
>>> >> > pg_stat_bgwriter view also.
>>> >> >
>>> >>
>>> >> I feel separate view is better.
>>> >
>>> >
>>> > Ok.
>>> >
>>> > Following the sample out of the view after regress run.
>>> >
>>> > postgres=# select * from pg_stat_walwrites;
>>> > -[ RECORD 1 ]--+--
>>> > backend_writes | 19092
>>> > writes | 663
>>> > write_blocks   | 56116
>>> > write_time | 0
>>> > sync_time  | 3064
>>> > stats_reset| 2017-02-15 13:37:09.454314+11
>>> >
>>> > Currently, writer, walwriter and checkpointer processes
>>> > are considered as background processes that can do
>>> > the wal write mainly.
>>
>>
>> Thanks for the review.
>>
>>> I'm not sure if this categorization is good. You told that this view is
>>> useful
>>> to tune walwriter parameters at the top of this thread. If so, ISTM that
>>> the information about walwriter's activity should be separated from
>>> others.
>>
>>
>> Yes, that's correct. First I thought of providing the statistics of
>> walwriter, but
>> later in development, it turned into showing statistics of all wal write
>> activity
>> of background processes also to differentiate the actual write by the
>> backends.
>>
>>>
>>> What about other processes which *can* write WAL, for example walsender
>>> (e.g., BASE_BACKUP can cause WAL record), startup process (e.g., end-of-
>>> recovery checkpoint) and logical replication worker (Not sure if it
>>> always
>>> works with synchronous_commit=off, though). There might be other
>>> processes
>>> which can write WAL
>>
>>
>> It is possible to add the walsender, stratup and other processes easily,
>> but not
>> background workers that does some wal write operations until unless they
>> report the stats with pgstat_report_stat(). Is it fine to ignore the
>> workers that
>> does not report the stats?
>
>
> Added stats collection for walsender, statrup and autovacuum processes.
> The background workers that call pgstat_report_stat() function will
> automatically
> included.

So you added pgstat_send_walwrites() into several places. But WAL activity
by the background workers which don't call pgstat_report_stat() is still not
considered in pg_stat_walwrites view. This design looks fragile to me.
I think that we should capture all the WAL activities (even by such background
workers) in XLogWrite(), instead.

For example, we can add the "shared" counters (protected by WALWriteLock)
into XLogCtl and make XLogWrite() update those counters if the process is

Re: [HACKERS] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 9:47 AM, Dave Page  wrote:
> 
>> Does
>> pg_read_all_stats still have access to stats for mysecuretable?
> 
> Yes, because the ACL on the table controls reading/writing the data in
> the table. It doesn't have any bearing on any kind of table metadata.
> A user who has no privileges on a table can already look at (for
> example) pg_stat_all_tables and see the sort of info you're talking
> about. This patch would just allow members of a specific role get the
> on-disk size as well, *if* you choose to use it.

I don't entirely agree here.  Security conscious database users may well
restrict access to that view.  I added a check to the regression tests to
verify that it works:

+ SET ROLE regress_locktable_user;
+ SELECT COUNT(*) FROM pg_stat_all_tables;
+  count 
+ ---
+290
+ (1 row)
+ 
+ RESET ROLE;
+ REVOKE ALL PRIVILEGES ON pg_stat_all_tables FROM PUBLIC;
+ SET ROLE regress_locktable_user;
+ SELECT COUNT(*) FROM pg_stat_all_tables;
+ ERROR:  permission denied for relation pg_stat_all_tables
+ RESET ROLE;
+ SELECT COUNT(*) FROM pg_stat_all_tables;
+  count 
+ ---
+292
+ (1 row)
+ 

The inability to revoke access to this sort of information being proposed
makes me a bit uneasy.  Mostly, I think, I'm bothered because there may
be people who have revoked privileges on a lot of things, thereby restricting
access to superuser, who won't necessarily notice this new feature in
postgres 10.  That could be a source of security holes that we get blamed
for.

Please note that I'm not specifically opposed to this work, and see a lot
of merit here.  I'm just thinking about unintended consequences.

mark



-- 
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] Monitoring roles patch

[Dave Page]

On Tue, Mar 28, 2017 at 1:52 PM, Mark Dilger  wrote:
>
>> On Mar 28, 2017, at 9:55 AM, Robert Haas  wrote:
>>
>> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
 I don't see any precedent in the code for having a hardcoded role, other 
 than
 superuser, and allowing privileges based on a hardcoded test for membership
 in that role.  I'm struggling to think of all the security implications of 
 that.
>>>
>>> This would be the first.
>>
>> Isn't pg_signal_backend an existing precedent?
>
> Sorry, I meant to say that there is no precedent for allowing access to data 
> based
> on a hardcoded test for membership in a role other than superuser.

This doesn't allow access to data, except through monitoring of
queries that are executed (e.g. full access to pg_stat_activity) -
which you can avoid by not using the role if that's your choice.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] Supporting huge pages on Windows

[David Steele]

On 3/24/17 12:51 PM, Ashutosh Sharma wrote:

Hi,

On Fri, Mar 24, 2017 at 9:00 PM, David Steele  wrote:

Hi Ashutosh,

On 3/22/17 8:52 AM, Amit Kapila wrote:


On Wed, Mar 22, 2017 at 12:07 AM, David Steele 
wrote:



Amit, Magnus, you are signed up as reviewers for this patch.  Do you know
when you'll have a chance to take a look?



I have provided my feedback and I could not test it on my machine.  I
think Ashutosh Sharma has tested it.  I can give it another look, but
not sure if it helps.



I know you are not signed up as a reviewer, but have you take a look at this
patch?


Well, I had a quick look into the patch just because I wanted the test
it as I am having windows setup. But, I never looked into the patch
from reviewer's perspective. The intention was to reverify the test
results shared by Tsunawaka.


Thanks, Ashutosh.

It's not clear to me what state this patch should be in.  Is there more 
review that needs to be done or is it ready for a committer?


--
-David
da...@pgmasters.net


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

[HACKERS] Protection lost in expression eval changeover

[Tom Lane]

CheckVarSlotCompatibility contains the comment

 * Note: we allow a reference to a dropped attribute.  slot_getattr will
 * force a NULL result in such cases.

While still true, that second sentence is now quite irrelevant, because we
don't go through slot_getattr anymore.  So it seems like we are missing
some needed protection.  I'm inclined to think that it'd be all right to
just throw an error immediately in CheckVarSlotCompatibility if the
target column is dropped.

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] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 9:55 AM, Robert Haas  wrote:
> 
> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
>>> I don't see any precedent in the code for having a hardcoded role, other 
>>> than
>>> superuser, and allowing privileges based on a hardcoded test for membership
>>> in that role.  I'm struggling to think of all the security implications of 
>>> that.
>> 
>> This would be the first.
> 
> Isn't pg_signal_backend an existing precedent?

Sorry, I meant to say that there is no precedent for allowing access to data 
based
on a hardcoded test for membership in a role other than superuser.  All the
locations that use pg_signal_backend are checking for something other than
data access privileges.  That distinction was clear to me in the context of 
what I
was saying, but I obviously didn't phrase it right in my email.

mark

-- 
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] Monitoring roles patch

[Dave Page]

OK, so before I start hacking again, here's a proposal based on my
understanding of folks comments, and so open questions. If I can get
agreement and answers, I'll be able to break out vi again without
(hopefully) too many more revisions:

pg_read_all_stats: Will have C-coded access to pg_stats views and
pg_*_size that are currently hard-coded to superuser

pg_read_all_settings: Will have C-coded access to read GUCs that are
currently hard-coded to the superuser

pg_monitor: Will include pg_read_all_stats and pg_read_all_settings,
and all explicitly GRANTable rights, e.g. in contrib modules.

Patch to be rebased on Simon's updated version.

Questions:

- pg_stat_statements has a hard-coded superuser check. Shall I remove
that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor?

- pgrowlocks has hard-coded access to superuser and users with SELECT
on the table being examined. How should this be handled?

- Stephen suggested a separate role for functions that can lock
tables. Is this still desired, or shall we just grant access to
pg_monitor (I think the latter is fine)?

- Based on Peter's concerns, is pg_read_all_stats the right name?
Maybe pg_read_monitoring_stats?

Thanks!

On Tue, Mar 28, 2017 at 1:37 PM, Stephen Frost  wrote:
> Greetings,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
>> >> I don't see any precedent in the code for having a hardcoded role, other 
>> >> than
>> >> superuser, and allowing privileges based on a hardcoded test for 
>> >> membership
>> >> in that role.  I'm struggling to think of all the security implications 
>> >> of that.
>> >
>> > This would be the first.
>>
>> Isn't pg_signal_backend an existing precedent?
>
> Yes, it is.
>
> Thanks!
>
> Stephen



-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

[Pavel Stehule]

Hi

rebased due last changes in pg_exec.c

Regards

Pavel
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index d356deb9f5..56da4d6163 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -802,6 +802,32 @@ $$ LANGUAGE plpgsql;
 happen in a plain SQL command.

   
+
+  
+   Block level PRAGMA
+
+   
+PRAGMA
+in PL/pgSQL
+   
+
+   
+The block level PRAGMA allows to change some
+PL/pgSQL compiler behave. Currently
+only PRAGMA PLAN_CACHE is supported.
+
+
+CREATE FUNCTION enforce_fresh_plan(_id text) RETURNS boolean AS $$
+DECLARE
+  PRAGMA PLAN_CACHE(force_custom_plan);
+BEGIN
+  -- in this block every embedded query uses one shot plan
+  RETURN EXISTS(SELECT * FROM tab WHERE id = _id);
+END;
+$$ LANGUAGE plpgsql;
+
+   
+  
   
 
   
@@ -4649,6 +4675,43 @@ $$ LANGUAGE plpgsql;
  use of the now() function would still be a better idea.
 
 
+
+
+ PRAGMA PLAN_CACHE
+
+ 
+  The plan cache behave can be controlled with PRAGMA PLAN_CACHE.
+  This PRAGMA can be used on function or on block level (per
+  function, per block). The following options are possible:
+  DEFAULT - default PL/pgSQL
+  implementation - the system try to decide between custom plan and generic
+  plan after five query executions, FORCE_CUSTOM_PLAN
+  - the execution plan is one shot plan - it is specific for every set of
+  used paramaters, FORCE_GENERIC_PLAN - the query plan
+  is generic from start.
+ 
+
+ 
+  
+   PRAGMA PLAN_CACHE
+   in PL/pgSQL
+  
+  The plan for INSERT is generic from begin. The 
+  PRAGMA PLAN_CACHE is related to function - etc. every command
+  in this function will use generic plan.
+
+CREATE FUNCTION logfunc2(logtxt text) RETURNS void AS $$
+PRAGMA PLAN_CACHE(FORCE_GENERIC_PLAN);
+DECLARE
+curtime timestamp;
+BEGIN
+curtime := 'now';
+INSERT INTO logtable VALUES (logtxt, curtime);
+END;
+$$ LANGUAGE plpgsql;
+
+ 
+
   
 
   
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index bed343ea0c..70c970d20c 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -85,6 +85,10 @@ static const ExceptionLabelMap exception_label_map[] = {
 	{NULL, 0}
 };
 
+PLpgSQL_settings default_settings = {
+	NULL,
+	0			/* no special cursor option */
+};
 
 /* --
  * static prototypes
@@ -371,6 +375,7 @@ do_compile(FunctionCallInfo fcinfo,
 	 * outermost namespace contains function parameters and other special
 	 * variables (such as FOUND), and is named after the function itself.
 	 */
+	plpgsql_settings_init(_settings);
 	plpgsql_ns_init();
 	plpgsql_ns_push(NameStr(procStruct->proname), PLPGSQL_LABEL_BLOCK);
 	plpgsql_DumpExecTree = false;
@@ -849,6 +854,7 @@ plpgsql_compile_inline(char *proc_source)
 	function->extra_warnings = 0;
 	function->extra_errors = 0;
 
+	plpgsql_settings_init(_settings);
 	plpgsql_ns_init();
 	plpgsql_ns_push(func_name, PLPGSQL_LABEL_BLOCK);
 	plpgsql_DumpExecTree = false;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index c27935b51b..762ee6f70f 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2336,7 +2336,7 @@ exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
 	Assert(query);
 
 	if (query->plan == NULL)
-		exec_prepare_plan(estate, query, curvar->cursor_options);
+		exec_prepare_plan(estate, query, query->cursor_options);
 
 	/*
 	 * Set up short-lived ParamListInfo
@@ -3626,7 +3626,8 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
 	{
 		ListCell   *l;
 
-		exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+		exec_prepare_plan(estate, expr,
+		  expr->cursor_options | CURSOR_OPT_PARALLEL_OK);
 		stmt->mod_stmt = false;
 		foreach(l, SPI_plan_get_plan_sources(expr->plan))
 		{
@@ -4096,7 +4097,8 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
 		 */
 		query = stmt->query;
 		if (query->plan == NULL)
-			exec_prepare_plan(estate, query, stmt->cursor_options);
+			exec_prepare_plan(estate, query,
+			  query->cursor_options | stmt->cursor_options);
 	}
 	else if (stmt->dynquery != NULL)
 	{
@@ -4167,7 +4169,7 @@ exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
 
 		query = curvar->cursor_explicit_expr;
 		if (query->plan == NULL)
-			exec_prepare_plan(estate, query, curvar->cursor_options);
+			exec_prepare_plan(estate, query, query->cursor_options);
 	}
 
 	/*
@@ -4366,7 +4368,7 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
 	 */
 	if (expr->plan == NULL)
 	{
-		exec_prepare_plan(estate, expr, 0);
+		exec_prepare_plan(estate, expr, expr->cursor_options);
 		if (target->dtype == PLPGSQL_DTYPE_VAR)
 			exec_check_rw_parameter(expr, target->dno);
 	}
@@ -5174,7 +5176,8 @@ exec_eval_expr(PLpgSQL_execstate *estate,
 	 * If first time through, create a plan for this expression.
 	 */
 	if 

Re: [HACKERS] Removing binaries

[Tom Lane]

Robert Haas  writes:
> On Tue, Mar 28, 2017 at 12:18 PM, Tom Lane  wrote:
>> Perhaps we could satisfy novices by changing the out-of-the-box
>> behavior, but provide some way to select the old behavior for
>> installations that are really depending on it.

> Hmm.  I guess that would mean that the same connection string would
> mean something different depending on how you configured this
> behavior, which does not sound like a good idea.  But why not go the
> other way and just create the default database by default, either in
> addition to or instead of the postgres database?

What "default database"?  Are you suggesting that createuser should
forcibly create a similarly-named database?  Doesn't sound like a
great idea to me; it's presuming what seems like minority usage.

If we were to do anything about this, my sketch of a desirable solution
would be to invent a GUC named something like "default_database", which
would specify the DB to connect to when the incoming request packet
doesn't name one.  We could have it be "postgres" out of the box, but
allow the value "$user" to select the old behavior.

However ... a little bit of research shows that this behavior is wired
into libpq as well as the backend, and that therefore the backend's
code path that inserts the username as the default DB name is dead
code (at least when processing libpq-driven connection requests;
I don't know what JDBC and other client libraries do).  This means
that there isn't any really centralized way to fix it.  We could
delete that behavior from libpq easily enough, but then we'd have
a situation where the behavior varies depending on which version of
which client library you're using.

I'm afraid now that changing this would be far more pain than it
could possibly be worth.

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] Monitoring roles patch

[Stephen Frost]

Greetings,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
> >> I don't see any precedent in the code for having a hardcoded role, other 
> >> than
> >> superuser, and allowing privileges based on a hardcoded test for membership
> >> in that role.  I'm struggling to think of all the security implications of 
> >> that.
> >
> > This would be the first.
> 
> Isn't pg_signal_backend an existing precedent?

Yes, it is.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] GSoC 2017 Proposal for "Explicitly support predicate locks in index access methods besides btree"

[Shubham Barai]

Hi guys,

My name is Shubham Barai and I am a final year student at Maharashtra
Institute of Technology, Pune, India. I am very interested in contributing
Postgresql this year through GSoC project.

I am particularly interested in working on the project "Explicitly support
predicate locks in index access methods besides btree". I have gone through
some research papers which were recommended on https://wiki.postgresql.
org/wiki/GSoC_2017 to understand the concept of Serializable Snapshot
Isolation used in PostgreSQL. I have also browsed through the codebase to
get some idea of different access methods for gin, gist, and hash indexes.
I want to discuss my proposal to get some feedback before I write my final
proposal. Sorry, I am discussing my proposal little late. I was really busy
in my academics.

Currently, only B+-trees support page level predicate locking.For other
indexes, it acquires relation level lock which can lead to unnecessary
serialization failure due to rw dependency caused by any insert into the
index. So, the main task of this project is to support page level predicate
locking for remaining indexes.

* Gist Index

B+tree index acquires predicate locks only on leaf pages as index tuples
with record pointers are stored on leaf pages. But for Gist index, we have
to consider locking at each index level as index tuples can be stored in
buffers associated with internal pages or in leaf pages.
So, the functions where we need to insert a call for

1. PredicateLockPage()

->gistScanPage()
after acquiring a shared lock on a buffer

2.CheckForSerializableConflictIn()

-> gistdoinsert()
after acquiring an exclusive lock on a target buffer and before inserting a
tuple


3. PredicateLockPageSplit()

->gistplacetopage()

If there is not enough space for insertion, we need to copy predicate lock
from an old page to all new pages generated after a successful split
operation.

PredicateLockPageSplit(Relation relation, BlockNumber oldblkno, BlockNumber
a lot) is used by b+-tree where two pages are involved in a split
operation. For  Gist index, we can define a similar function where more
than one page can be generated after split operation.




* Gin Index

Gin index consists of a B-tree index over key values, where each key is an
element of some indexed items and each tuple in a leaf page contains either
a posting list if the list is small enough or a pointer to posting tree.

1. PredicateLockPage()

->startscanentry()
   before calling collectMatchBitmap()

->scanPostingTree()
   after acquiring a shared lock on a leaf page

2.CheckForSerializableConflictIn()

-> ginentryinsert()

->gininsertitempointers()
   in case of insertion in a posting tree


3. PredicateLockPageSplit()

-> dataBeginPlacetoPageLeaf()

 after calling dataPlacetoPageLeafSplit()


* Hash Index

1. PredicateLockPage()

->hashgettuple()
->_hash_first()
->_hash_readnext()
->_hash_readprev()

2.CheckForSerializableConflictIn()

-> _hash_doinsert()

3. PredicateLockPageSplit()

currently, I am trying to understand how the splitting of bucket works.
should we acquire predicate lock on every page from an old and new bucket
in case there is a predicate lock on a page of an old bucket?

There may be a lot of other places where we need to insert function calls
for predicate locking that I haven't included yet. I didn't go into details
of every index AM.

can anyone help me find existing tests for b-tree?


Regards,
Shubham

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Tue, Mar 28, 2017 at 8:34 PM, Robert Haas  wrote:

> On Mon, Mar 27, 2017 at 10:25 PM, Pavan Deolasee
>  wrote:
> > On Tue, Mar 28, 2017 at 1:59 AM, Robert Haas 
> wrote:
> >> On Thu, Mar 23, 2017 at 2:47 PM, Pavan Deolasee
> >>  wrote:
> >> > It's quite hard to say that until we see many more benchmarks. As
> author
> >> > of
> >> > the patch, I might have got repetitive with my benchmarks. But I've
> seen
> >> > over 50% improvement in TPS even without chain conversion (6 indexes
> on
> >> > a 12
> >> > column table test).
> >>
> >> This seems quite mystifying.  What can account for such a large
> >> performance difference in such a pessimal scenario?  It seems to me
> >> that without chain conversion, WARM can only apply to each row once
> >> and therefore no sustained performance improvement should be possible
> >> -- unless rows are regularly being moved to new blocks, in which case
> >> those updates would "reset" the ability to again perform an update.
> >> However, one would hope that most updates get done within a single
> >> block, so that the row-moves-to-new-block case wouldn't happen very
> >> often.
> >
> > I think you're confusing between update chains that stay within a block
> vs
> > HOT/WARM chains. Even when the entire update chain stays within a block,
> it
> > can be made up of multiple HOT/WARM chains and each of these chains offer
> > ability to do one WARM update. So even without chain conversion, every
> > alternate update will be a WARM update. So the gains are perpetual.
>
> You're right, I had overlooked that.  But then I'm confused: how does
> the chain conversion stuff help as much as it does?  You said that you
> got a 50% improvement from WARM, because we got to skip half the index
> updates.  But then you said with chain conversion you got an
> improvement of more like 100%.  However, I would think that on this
> workload, chain conversion shouldn't save much.  If you're sweeping
> through the database constantly performing updates, the updates ought
> to be a lot more frequent than the vacuums.
>
> No?


These tests were done on a very large table of 80M rows. The table itself
was wide with 15 columns and a few indexes. So in a 8hr test, master could
do only 55M updates where as WARM did 105M updates. There were 4 autovacuum
cycles in both these runs. So while there were many updates, I am sure
autovacuum must have helped to increase the percentage of WARM updates
(from ~50% after steady state to ~67% after steady state). Also I said more
than 50%, but it was probably close to 65%.

Unfortunately these tests were done on different hardware, with different
settings and even slightly different scale factors. So they may not be
exactly comparable. But there is no doubt chain conversion will help to
some extent. I'll repeat the benchmark with chain conversion turned off and
report the exact difference.

Thanks,
Pavan

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

Re: [HACKERS] [COMMITTERS] pgsql: Allow vacuums to report oldestxmin

[Alvaro Herrera]

Fujii Masao wrote:

> This is the evidence that no one cares about the details of VACUUM VERBOSE
> output example. So I'm tempted to simplify the example (please see the
> attached patch) instead of keeping updating the example.

Agreed.

> > If this patch is applied, it should back-patch to all supported
> > branches.
> 
> Not sure if it's worth back-patching that.

I see no reason to keep it in back branches either.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Tue, Mar 28, 2017 at 7:04 PM, Amit Kapila 
wrote:

>
>
>   For such an heap insert, we will pass
> the actual value of column to index_form_tuple during index insert.
> However during recheck when we fetch the value of c2 from heap tuple
> and pass it index tuple, the value is already in compressed form and
> index_form_tuple might again try to compress it because the size will
> still be greater than TOAST_INDEX_TARGET and if it does so, it might
> make recheck fail.
>
>
Would it? I thought  "if
(!VARATT_IS_EXTENDED(DatumGetPointer(untoasted_values[i]))" check should
prevent that. But I could be reading those macros wrong. They are probably
heavily uncommented and it's not clear what each of those VARATT_* macro do.

Thanks,
Pavan

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

Re: [HACKERS] Monitoring roles patch

[Dave Page]

On Tue, Mar 28, 2017 at 12:55 PM, Robert Haas  wrote:
> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
>>> I don't see any precedent in the code for having a hardcoded role, other 
>>> than
>>> superuser, and allowing privileges based on a hardcoded test for membership
>>> in that role.  I'm struggling to think of all the security implications of 
>>> that.
>>
>> This would be the first.
>
> Isn't pg_signal_backend an existing precedent?

Good point. Clearly time for some caffeine.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Tue, Mar 28, 2017 at 4:07 PM, Amit Kapila 
wrote:

>
> Noted few cosmetic issues in 0005_warm_updates_v21:
>
> 1.
> pruneheap.c(939): warning C4098: 'heap_get_root_tuples' : 'void'
> function returning a value
>

Thanks. Will fix.


>
> 2.
> + *  HCWC_WARM_UPDATED_TUPLE - a tuple with HEAP_WARM_UPDATED is found
> somewhere
> + *in the chain. Note that when a tuple is WARM
> + *updated, both old and new versions are marked
> + *with this flag/
> + *
> + *  HCWC_WARM_TUPLE  - a tuple with HEAP_WARM_TUPLE is found somewhere in
> + *  the chain.
> + *
> + *  HCWC_CLEAR_TUPLE - a tuple without HEAP_WARM_TUPLE is found somewhere
> in
> + *   the chain.
>
> Description of all three flags is same.
>
>
Well the description is different (and correct), but given that it confused
you, I think I should rewrite those comments. Will do.


> 3.
> + *  HCWC_WARM_UPDATED_TUPLE - a tuple with HEAP_WARM_UPDATED is found
> somewhere
> + *in the chain. Note that when a tuple is WARM
> + *updated, both old and new versions are marked
> + *with this flag/
>
> Spurious '/' at end of line.
>
>
Thanks. Will fix.

Thanks,
Pavan

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

Re: [HACKERS] Patch: Write Amplification Reduction Method (WARM)

[Pavan Deolasee]

On Tue, Mar 28, 2017 at 4:05 PM, Amit Kapila 
wrote:

>
>
> As asked previously, can you explain me on what basis are you
> considering it robust?  The comments on top of datumIsEqual() clearly
> indicates the danger of using it for toasted values (Also, it will
> probably not give the answer you want if either datum has been
> "toasted".).


Hmm. I don' see why the new code in recheck is unsafe. The index values
themselves can't be toasted (IIUC), but they can be compressed.
index_form_tuple() already untoasts any toasted heap attributes and
compresses them if needed. So once we pass heap values via
index_form_tuple() we should have exactly the same index values as they
were inserted. Or am I missing something obvious here?


  If you think that because we are using it during
> heap_update to find modified columns, then I think that is not right
> comparison, because there we are comparing compressed value (of old
> tuple) with uncompressed value (of new tuple) which should always give
> result as false.
>
>
Hmm, this seems like a problem. While HOT could tolerate occasional false
results (i.e. reporting a heap column as modified even though it it not),
WARM assumes that if the heap has reported different values, then they
better be different and should better result in different index values.
Because that's how recheck later works. Since index expressions are not
supported, I wonder if toasted heap values are the only remaining problem
in this area. So heap_tuple_attr_equals() should first detoast the heap
values and then do the comparison. I already have a test case that fails
for this reason, so let me try this approach.

Thanks,
Pavan

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

Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

[Alvaro Herrera]

Nikolay Shaplov wrote:
> В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал:
> 
> > Please make sure to mark functions as static (e.g. bringetreloptcatalog).
> I am a bit confused here:
> 
> For brin and nbtree this is a good idea: brin.c and nbtree.c has AM-handler 
> inside it, and  there are other static functions there. Adding one more 
> static 
> function here seems to be quite logical.

I am just saying that if there is a function used only in one
compilation unit (.c file) then let's make sure its prototype says
"static".

> For gin, gist and spgist, authors seems to use [index_name]_private.h files 
> to 
> hide internal functions from outside code. In ginutil.c and spgutils.c, where 
> AM-handler is located, there is no static functions at all...  gist.c has, 
> but 
> I think I should write similar code for all *_private.h indexes. 

Sure.

> hash.c is quite a mess...
> There is no hash_private.h, AM-handles is located in hash.c, that has "This 
> file contains only the public interface routines." comment at the beginning, 
> and there is no static functions inside. I do not know what is the right way 
> to hide hashgetreloptcatalog function here...
> 
> What would you advise?

Leave it where it is (hashutil.c), no static marker.  We may want to
move things around later, but that's not your patch's responsibility.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Monitoring roles patch

[Robert Haas]

On Tue, Mar 28, 2017 at 12:47 PM, Dave Page  wrote:
>> I don't see any precedent in the code for having a hardcoded role, other than
>> superuser, and allowing privileges based on a hardcoded test for membership
>> in that role.  I'm struggling to think of all the security implications of 
>> that.
>
> This would be the first.

Isn't pg_signal_backend an existing precedent?

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


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

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Mon, Mar 27, 2017 at 8:36 AM, Ashutosh Bapat
 wrote:
> I have gone through the patch, and it looks good to me. Here's the set
> of patches with this patch included. Fixed the testcase failures.
> Rebased the patchset on de4da168d57de812bb30d359394b7913635d21a9.

This version of 0001 looks much better to me, but I still have some concerns.

I think we should also introduce IS_UPPER_REL() at the same time, for
symmetry and because partitionwise aggregate will need it, and use it
in place of direct tests against RELOPT_UPPER_REL.

I think it would make sense to change the test in deparseFromExpr() to
check for IS_JOIN_REL() || IS_SIMPLE_REL().  There's no obvious reason
why that shouldn't be OK, and it would remove the last direct test
against RELOPT_JOINREL in the tree, and it will probably need to be
changed for partitionwise aggregate anyway.

Could set_append_rel_size Assert(IS_SIMPLE_REL(rel))?  I notice that
you did this in some other places such as
generate_implied_equalities_for_column(), and I like that.  If for
some reason that's not going to work, then it's doubtful whether
Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL) is going to
survive either.

Similarly, I think relation_excluded_by_constraints() would also
benefit from Assert(IS_SIMPLE_REL(rel)).

Why not set top_parent_relids earlier, when actually creating the
RelOptInfo?  I think you could just change build_simple_rel() so that
instead of passing RelOptKind reloptkind, you instead pass RelOptInfo
*parent.  I think postponing that work until set_append_rel_size()
just introduces possible bugs resulting from it not being set early
enough.

Apart from the above, I think 0001 is in good shape.

Regarding 0002, I think the parts that involve factoring out
find_param_path_info() are uncontroversial.  Regarding the changes to
adjust_appendrel_attrs(), my main question is whether we wouldn't be
better off using an array representation rather than a List
representation.  In other words, this function could take PlannerInfo
*root, Node *node, int nappinfos, AppendRelInfo **appinfos.  Existing
callers doing adjust_appendrel_attrs(root, whatever, appinfo) could
just do adjust_appendrel_attrs(root, whatever, 1, ), not
needing to allocate.  To make this work, adjust_child_relids() and
find_appinfos_by_relids() would need to be adjusted to use a similar
argument-passing convention.  I suspect this makes iterating over the
AppendRelInfos mildly faster, too, apart from the memory savings.

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


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

Re: [HACKERS] Monitoring roles patch

[Dave Page]

On Tue, Mar 28, 2017 at 12:04 PM, Mark Dilger  wrote:
>
>> On Mar 28, 2017, at 8:34 AM, Dave Page  wrote:
>>
>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut
>>  wrote:
>>> This patch touches the pg_buffercache and pg_freespacemap extensions,
>>> but there appear to be some files missing.
>>
>> Are you looking at an old version? There was one where I forgot to add
>> some files, but that was fixed within an hour or so in a new version.
>>
>> Right now I'm waiting for discussion to conclude before updating the
>> patch again.
>
> There does not seem to be a new patch since Robert made his "modest proposal",
> so I guess I just have to ask questions about how this would work.

There hasn't been one yet.

> I don't see any precedent in the code for having a hardcoded role, other than
> superuser, and allowing privileges based on a hardcoded test for membership
> in that role.  I'm struggling to think of all the security implications of 
> that.

This would be the first.

> If I have even one table in my database which is security sensitive, such that
> I cannot allow users to see the size of the table, nor whether the table has
> unvacuumed rows (owing to the fact that would give away that it has been
> changed since the last vacuum time), then I can't use pg_real_all_stats for
> anything, right?  And I would need to exercise some due diligence to make
> certain it does not get granted to anybody?

Right.

> What happens if I execute:
>
> REVOKE ALL ON TABLE mysecuretable FROM pg_read_all_stats?
>
> Does it work?

Yes, for the documented use of GRANT/REVOKE on a table.

> Does it silently fail?  Does it raise an exception?

No and no.

> Does
> pg_read_all_stats still have access to stats for mysecuretable?

Yes, because the ACL on the table controls reading/writing the data in
the table. It doesn't have any bearing on any kind of table metadata.
A user who has no privileges on a table can already look at (for
example) pg_stat_all_tables and see the sort of info you're talking
about. This patch would just allow members of a specific role get the
on-disk size as well, *if* you choose to use it.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] Removing binaries

[Robert Haas]

On Tue, Mar 28, 2017 at 12:18 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Mar 28, 2017 at 11:44 AM, Peter Eisentraut
>>  wrote:
>>> On 3/21/17 08:12, Robert Haas wrote:
 I think a big part of the usability problem here comes from the fact
 that the default database for connections is based on the username,
 but the default databases that get created have fixed names (postgres,
 template1).  So the default configuration is one where you can't
 connect.  Why the heck do we do it that way?
>
>>> Historical, probably.  We could ponder changing the way the default
>>> database is determined, but I don't want to imagine the breakage coming
>>> out of that.
>
>> What do you think would break?
>
> Any configuration depending on the existing default?
>
> The existing behavior here dates from before we had schemas, so that
> if users wanted to have private objects they *had* to use separate
> databases.  Nowadays a schema-per-user within one database makes a lot
> more sense for many environments, and we even have the default value
> for search_path set up to make that as painless as possible.  Still,
> it's not a solution for everybody, particularly not installations
> that want to keep their users well separated.
>
> Perhaps we could satisfy novices by changing the out-of-the-box
> behavior, but provide some way to select the old behavior for
> installations that are really depending on it.

Hmm.  I guess that would mean that the same connection string would
mean something different depending on how you configured this
behavior, which does not sound like a good idea.  But why not go the
other way and just create the default database by default, either in
addition to or instead of the postgres database?  I mean, most people
probably do this:

initdb
pg_ctl start
createdb

If initdb created the database that you currently have to create as a
separate step by running 'createdb', I bet we'd eliminate a metric
buttload of new user confusion and harm almost nobody.  Anybody who
doesn't want that extra database can just drop it.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Allow vacuums to report oldestxmin

[Fujii Masao]

On Wed, Mar 29, 2017 at 1:32 AM, Fujii Masao  wrote:
> On Tue, Mar 28, 2017 at 1:06 AM, Masahiko Sawada  
> wrote:
>> On Sun, Mar 26, 2017 at 2:26 AM, Masahiko Sawada  
>> wrote:
>>> On Sun, Mar 26, 2017 at 1:37 AM, Fujii Masao  wrote:
 On Mon, Mar 6, 2017 at 9:37 PM, Masahiko Sawada  
 wrote:
> On Fri, Mar 3, 2017 at 10:50 PM, Simon Riggs  
> wrote:
>> Allow vacuums to report oldestxmin
>>
>> Allow VACUUM and Autovacuum to report the oldestxmin value they
>> used while cleaning tables, helping to make better sense out of
>> the other statistics we report in various cases.
>>
>> Branch
>> --
>> master
>>
>> Details
>> ---
>> http://git.postgresql.org/pg/commitdiff/9eb344faf54a849898d9be012ddfa8204cfeb57c
>>
>> Modified Files
>> --
>> src/backend/commands/vacuumlazy.c | 9 +
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>>
>
> Should we change the example in vacuum.sgml file as well? Attached patch.

 "tuples" in the above should be "row versions"?
 We should review not only this line but also all the lines in the example
 of VERBOSE output, I think.
>>>
>>> Right. These verbose log messages are out of date. I ran
>>> VACUUM(VERBOSE, ANALYZE) with same scenario as current example as
>>> possible. Attached patch updates verbose log messages.
>>>
>>>
>>
>> Surprisingly the changes "tuples" -> "row versions" in vacuumlazy.c is
>> introduced by commit feb4f44d296b88b7f0723f4a4f3945a371276e0b in 2003.
>
> This is the evidence that no one cares about the details of VACUUM VERBOSE
> output example. So I'm tempted to simplify the example (please see the
> attached patch) instead of keeping updating the example.

Patch attached.

Regards,

-- 
Fujii Masao


vacuum-example.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] [COMMITTERS] pgsql: Allow vacuums to report oldestxmin

[Fujii Masao]

On Tue, Mar 28, 2017 at 1:06 AM, Masahiko Sawada  wrote:
> On Sun, Mar 26, 2017 at 2:26 AM, Masahiko Sawada  
> wrote:
>> On Sun, Mar 26, 2017 at 1:37 AM, Fujii Masao  wrote:
>>> On Mon, Mar 6, 2017 at 9:37 PM, Masahiko Sawada  
>>> wrote:
 On Fri, Mar 3, 2017 at 10:50 PM, Simon Riggs  wrote:
> Allow vacuums to report oldestxmin
>
> Allow VACUUM and Autovacuum to report the oldestxmin value they
> used while cleaning tables, helping to make better sense out of
> the other statistics we report in various cases.
>
> Branch
> --
> master
>
> Details
> ---
> http://git.postgresql.org/pg/commitdiff/9eb344faf54a849898d9be012ddfa8204cfeb57c
>
> Modified Files
> --
> src/backend/commands/vacuumlazy.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>

 Should we change the example in vacuum.sgml file as well? Attached patch.
>>>
>>> "tuples" in the above should be "row versions"?
>>> We should review not only this line but also all the lines in the example
>>> of VERBOSE output, I think.
>>
>> Right. These verbose log messages are out of date. I ran
>> VACUUM(VERBOSE, ANALYZE) with same scenario as current example as
>> possible. Attached patch updates verbose log messages.
>>
>>
>
> Surprisingly the changes "tuples" -> "row versions" in vacuumlazy.c is
> introduced by commit feb4f44d296b88b7f0723f4a4f3945a371276e0b in 2003.

This is the evidence that no one cares about the details of VACUUM VERBOSE
output example. So I'm tempted to simplify the example (please see the
attached patch) instead of keeping updating the example.

> If this patch is applied, it should back-patch to all supported
> branches.

Not sure if it's worth back-patching that.

Regards,

-- 
Fujii Masao


-- 
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] On How To Shorten the Steep Learning Curve Towards PG Hacking...

[Adrien Nayrat]

On 03/27/2017 02:00 PM, Kang Yuzhe wrote:
> 1. Prepare Hands-on with PG internals
> 
>  For example, a complete Hands-on  with SELECT/INSERT SQL Standard PG 
> internals.
> The point is the experts can pick one fairly complex feature and walk it from
> Parser to Executor in a hands-on manner explaining step by step every 
> technical
> detail.
Hi,

Bruce Momjian has made several presentations about Postgres Internal :
http://momjian.us/main/presentations/internals.html


Regards
-- 
Adrien NAYRAT




signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Monitoring roles patch

[Dave Page]

On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut
 wrote:
> On 3/28/17 11:34, Dave Page wrote:
>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut
>>  wrote:
>>> This patch touches the pg_buffercache and pg_freespacemap extensions,
>>> but there appear to be some files missing.
>>
>> Are you looking at an old version? There was one where I forgot to add
>> some files, but that was fixed within an hour or so in a new version.
>
> What is the name and date or your latest patch?

pg_monitor_v4.diff, 23rd March.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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


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

Re: [HACKERS] Removing binaries

[Tom Lane]

Robert Haas  writes:
> On Tue, Mar 28, 2017 at 11:44 AM, Peter Eisentraut
>  wrote:
>> On 3/21/17 08:12, Robert Haas wrote:
>>> I think a big part of the usability problem here comes from the fact
>>> that the default database for connections is based on the username,
>>> but the default databases that get created have fixed names (postgres,
>>> template1).  So the default configuration is one where you can't
>>> connect.  Why the heck do we do it that way?

>> Historical, probably.  We could ponder changing the way the default
>> database is determined, but I don't want to imagine the breakage coming
>> out of that.

> What do you think would break?

Any configuration depending on the existing default?

The existing behavior here dates from before we had schemas, so that
if users wanted to have private objects they *had* to use separate
databases.  Nowadays a schema-per-user within one database makes a lot
more sense for many environments, and we even have the default value
for search_path set up to make that as painless as possible.  Still,
it's not a solution for everybody, particularly not installations
that want to keep their users well separated.

Perhaps we could satisfy novices by changing the out-of-the-box
behavior, but provide some way to select the old behavior for
installations that are really depending on it.

regards, tom lane


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

Re: [HACKERS] logical replication launcher crash on buildfarm

[Petr Jelinek]

On 28/03/17 17:55, Robert Haas wrote:
> On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
>  wrote:
>> On 28/03/17 04:46, Robert Haas wrote:
>>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund  wrote:
> Btw now that I look at the code, I guess we'll want to get rid of
> bgw_main completely in HEAD given that we can't guarantee it will be
> valid even for shared_preload_library libraries. For older branches I
> would leave things as they are in this regard as there don't seem to be
> any immediate issue for standard binaries.

 As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
 fine to just introduce bgw_builtin_id or such, and leave the bgw_main
 code in place in < HEAD.
>>>
>>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>>> along the lines of
>>>
>>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>>> {
>>> if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>>ParallelQueryMain(blah);
>>> else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>>LogicalReplicationMain(blah);
>>> }
>>>
>>> I think something like that is certainly better for the back-branches,
>>> because it doesn't cause an ABI break.  But I think it would also be
>>> fine for master.
>>>
>>
>> I had something slightly more complex like the attached in mind.
> 
> Seems broadly reasonable on a quick look, but I think we should leave
> bgw_main intact in 9.6.  It may be working for fine for people who
> don't care about Windows, and I'd rather not break it gratuitously.
> Can we have two patches, one of which converts the internal stuff to
> use the new mechanism and a second of which removes bgw_main?  The
> second one, at least, also needs to update the documentation.  (A good
> practice when removing an identifier is to run 'git grep
> thing_i_am_removing' after removing it...)
> 

Yes I agree (and I said it in the thread) I plan to submit the 9.6 too,
I just sent what I had before having it all ready mainly because you
started talking about code already and I didn't want you to waste time
with it needlessly, but it was already 5AM for me ;).

-- 
  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] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 8:34 AM, Dave Page  wrote:
> 
> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut
>  wrote:
>> This patch touches the pg_buffercache and pg_freespacemap extensions,
>> but there appear to be some files missing.
> 
> Are you looking at an old version? There was one where I forgot to add
> some files, but that was fixed within an hour or so in a new version.
> 
> Right now I'm waiting for discussion to conclude before updating the
> patch again.

There does not seem to be a new patch since Robert made his "modest proposal",
so I guess I just have to ask questions about how this would work.

I don't see any precedent in the code for having a hardcoded role, other than
superuser, and allowing privileges based on a hardcoded test for membership
in that role.  I'm struggling to think of all the security implications of that.

If I have even one table in my database which is security sensitive, such that
I cannot allow users to see the size of the table, nor whether the table has
unvacuumed rows (owing to the fact that would give away that it has been
changed since the last vacuum time), then I can't use pg_real_all_stats for
anything, right?  And I would need to exercise some due diligence to make
certain it does not get granted to anybody?

What happens if I execute:

REVOKE ALL ON TABLE mysecuretable FROM pg_read_all_stats?

Does it work?  Does it silently fail?  Does it raise an exception?  Does
pg_read_all_stats still have access to stats for mysecuretable?

mark



-- 
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] ALTER DEFAULT PRIVILEGES with GRANT/REVOKE ON SCHEMAS

[Teodor Sigaev]

Thank you, pushed

Matheus de Oliveira wrote:


On Thu, Mar 2, 2017 at 10:27 AM, David Steele > wrote:

It looks like this patch is still waiting on an update for tab
completion in psql.


Hi All,

Sorry about the long delay... It was so simple to add it to tab-complete.c that
is a shame I didn't do it before, very sorry about that.

Attached the new version of the patch that is basically the same as previously
with the addition to tab completion for psql and rebased with master.

Hope it is enough. Thank you all.

--
Matheus de Oliveira







--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM

[Nikolay Shaplov]

В письме от 17 марта 2017 14:21:26 пользователь Alvaro Herrera написал:

> Please make sure to mark functions as static (e.g. bringetreloptcatalog).
I am a bit confused here:

For brin and nbtree this is a good idea: brin.c and nbtree.c has AM-handler 
inside it, and  there are other static functions there. Adding one more static 
function here seems to be quite logical.

For gin, gist and spgist, authors seems to use [index_name]_private.h files to 
hide internal functions from outside code. In ginutil.c and spgutils.c, where 
AM-handler is located, there is no static functions at all...  gist.c has, but 
I think I should write similar code for all *_private.h indexes. 

So I think it wold be good to hide catalog function via *_pricate.h include 
file for these three indexes.

hash.c is quite a mess...
There is no hash_private.h, AM-handles is located in hash.c, that has "This 
file contains only the public interface routines." comment at the beginning, 
and there is no static functions inside. I do not know what is the right way 
to hide hashgetreloptcatalog function here...

What would you advise?


-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.


-- 
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] Today's failures on buildfarm member longfin

[Tom Lane]

Andres Freund  writes:
> On 2017-03-28 11:25:08 -0400, Tom Lane wrote:
>> Since, per previous agreement[1], longfin is running with -Werror, we
>> either have to do something about this or revert the decision to make it
>> use -Werror --- and I'm not too pleased about the latter idea.  We should
>> not have made that agreement if we were going to abandon it at the first
>> sign of pain.

> If necessary we could do that more targeted with
> -Wno-error=constant-conversion, but I think we should just fix this.

Yeah, that option seems like it would lose important error detection.

>> As noted in the other thread, we could either fix this in a
>> quick-and-dirty way by casting XLR_BLOCK_ID_DATA_SHORT and related
>> values to "char" explicitly, or we could run around and change the
>> target pointer variables to be "unsigned char *".

> I think both are ok with me.  Could also just use memcpy instead of
> direct assignment, but that seems too complicated.  I'd personally just
> go with Aleksander's casts.

Further investigation says that these warnings now also appear in the
9.5 and 9.6 branches, which were previously warning-clean except for
ye olde unused-variable flex bug.  So I'd like a solution that can be
back-patched, which says that the localized casts to char are the way to
go.  Maybe somebody else will want to undertake a more general cleanup,
but I don't want to spend time on that right now.

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] logical replication launcher crash on buildfarm

[Robert Haas]

On Mon, Mar 27, 2017 at 11:20 PM, Petr Jelinek
 wrote:
> On 28/03/17 04:46, Robert Haas wrote:
>> On Mon, Mar 27, 2017 at 10:04 PM, Andres Freund  wrote:
 Btw now that I look at the code, I guess we'll want to get rid of
 bgw_main completely in HEAD given that we can't guarantee it will be
 valid even for shared_preload_library libraries. For older branches I
 would leave things as they are in this regard as there don't seem to be
 any immediate issue for standard binaries.
>>>
>>> As long as you fix it so culicidae is happy (in 9.6) ;).  I think it's
>>> fine to just introduce bgw_builtin_id or such, and leave the bgw_main
>>> code in place in < HEAD.
>>
>> I wasn't thinking of introducing bgw_builtin_id.  My idea was just
>> along the lines of
>>
>> if (bgw_library_name == NULL && bgw_function_name != NULL)
>> {
>> if (strcmp(bgw_function_name, "ParallelQueryMain") == 0)
>>ParallelQueryMain(blah);
>> else if (strcmp(bgw_function_name, "LogicalReplicationMain") == 0)
>>LogicalReplicationMain(blah);
>> }
>>
>> I think something like that is certainly better for the back-branches,
>> because it doesn't cause an ABI break.  But I think it would also be
>> fine for master.
>>
>
> I had something slightly more complex like the attached in mind.

Seems broadly reasonable on a quick look, but I think we should leave
bgw_main intact in 9.6.  It may be working for fine for people who
don't care about Windows, and I'd rather not break it gratuitously.
Can we have two patches, one of which converts the internal stuff to
use the new mechanism and a second of which removes bgw_main?  The
second one, at least, also needs to update the documentation.  (A good
practice when removing an identifier is to run 'git grep
thing_i_am_removing' after removing it...)

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


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