Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks

[Mark Dilger]

> On Nov 10, 2017, at 3:58 PM, Stephen Frost  wrote:
> 
> Michael, Tom,
> 
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> On Fri, Nov 10, 2017 at 10:00 AM, Tom Lane  wrote:
>>> Stephen Frost  writes:
 I'm guessing no, which essentially means that *we* consider access to
 lo_import/lo_export to be equivilant to superuser and therefore we're
 not going to implement anything to try and prevent the user who has
 access to those functions from becoming superuser.  If we aren't willing
 to do that, then how can we really say that there's some difference
 between access to these functions and being a superuser?
>>> 
>>> We seem to be talking past each other.  Yes, if a user has malicious
>>> intentions, it's possibly to parlay lo_export into obtaining a superuser
>>> login (I'm less sure that that's necessarily true for lo_import).
>>> That does NOT make it "equivalent", except perhaps in the view of someone
>>> who is only considering blocking malevolent actors.  It does not mean that
>>> there's no value in preventing a task that needs to run lo_export from
>>> being able to accidentally destroy any data in the database.  There's a
>>> range of situations where you are concerned about accidents and errors,
>>> not malicious intent; but your argument ignores those use-cases.
>> 
>> That will not sound much as a surprise as I spawned the original
>> thread, but like Robert I understand that getting rid of all superuser
>> checks is a goal that we are trying to reach to allow admins to have
>> more flexibility in handling permissions to a subset of objects.
>> Forcing an admin to give full superuser rights to one user willing to
>> work only on LOs import and export is a wrong concept.
> 
> The problem I have with this is that 'full superuser rights' actually
> are being granted by this.  You're able to read any file and write any
> file on the filesystem that the PG OS user can.  How is that not 'full
> superuser rights'?  The concerns about a mistake being made are just as
> serious as they are with someone who has superuser rights as someone
> could pretty easily end up overwriting the control file or various other
> extremely important bits of the system.  This isn't just about what
> 'blackhats' can do with this.
> 
> As I mentioned up-thread, if we want to make it so that non-superusers
> can use lo_import/lo_export, then we should do that in a way that
> allows the administrator to specify exactly the rights they really want
> to give, based on a use-case for them.  There's no use-case for allowing
> someone to use lo_export() to overwrite pg_control.  The use-case would
> be to allow them to export to a specific directory (or perhaps a set of
> directories), or to read from same.  The same is true of COPY.  Further,
> I wonder what would happen if we allow this and then someone comes along
> and re-proposes the 'CREATE DIRECTORY' concept that I had once proposed
> (ideally with things cleaned up and tightened up to avoid there being
> issues using it) to address the actual use-case for these functions to
> be available to a non-superuser.  We wouldn't be able to change these
> functions to start checking the 'directory' rights or we would break
> existing non-superuser usage of them.  I imagine we could create
> additional functions which check the 'directory' privileges, but that's
> hardly ideal, imv.
> 
> I'm disappointed to apparently be in the minority on this, but that
> seems to be the case unless someone else wishes to comment.  While I
> appreciate the discussion, I'm certainly no more convinced today than I
> was when I first saw this go in that allowing these functions to be
> granted to non-superusers does anything but effectively make them into
> superusers who aren't explicitly identified as superusers.

Just to help understand your concern, and not to propose actual features,
would it help if there were

(1) a function, perhaps pg_catalog.pg_exploiters(), which would return all
roles that have been granted exploitable privileges, such that it would be
easier to identify all superusers, including those whose superuserishness
derives from a known export, and

(2) a syntax change for GRANT that would require an extra token, so
that you'd have to write something like

GRANT EXPLOITABLE lo_export TO trusted_user_foo

so that you couldn't unknowingly grant a dangerous privilege.

Or is there more to it than that?

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: multivariate histograms and MCV lists

[Mark Dilger]

> On Sep 12, 2017, at 2:06 PM, Tomas Vondra  
> wrote:
> 
> Attached is an updated version of the patch, dealing with fallout of
> 821fb8cdbf700a8aadbe12d5b46ca4e61be5a8a8 which touched the SGML
> documentation for CREATE STATISTICS.

Your patches need updating.

Tom's commit 471d55859c11b40059aef7dd82f82b3a0dc338b1 changed 
src/bin/psql/describe.c, which breaks your 0001-multivariate-MCV-lists.patch.gz
file.

I reviewed the patch a few months ago, and as I recall, it looked good to me.
I should review it again before approving it, though.

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] Add some const decorations to prototypes

[Mark Dilger]

> On Oct 31, 2017, at 7:46 AM, Peter Eisentraut 
>  wrote:
> 
> Here is a patch that adds const decorations to many char * arguments in
> functions.  It should have no impact otherwise; there are very few code
> changes caused by it.  Some functions have a strtol()-like behavior
> where they take in a const char * and return a pointer into that as
> another argument.  In those cases, I added a cast or two.
> 
> Generally, I find these const decorations useful as easy function
> documentation.

+1

I submitted something similar a while back and got into a back and forth
discussion with Tom about it.  Anyone interested could take a look at

https://www.postgresql.org/message-id/ACF3A030-E3D5-4E68-B744-184E11DE68F3%40gmail.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] proposal: schema variables

[Mark Dilger]

> Comments, notes?

How would variables behave on transaction rollback?

CREATE TEMP VARIABLE myvar;
SET myvar := 1;
BEGIN;
SET myvar := 2;
COMMIT;
BEGIN;
SET myvar := 3;
ROLLBACK;
SELECT myvar;

How would variables behave when modified in a procedure
that aborts rather than returning cleanly?


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] md5 still listed as an option in pg_hba.conf.sample

[Mark Dilger]

> On Sep 26, 2017, at 10:36 AM, Bruce Momjian <br...@momjian.us> wrote:
> 
> On Tue, Sep 26, 2017 at 10:23:55AM -0700, Mark Dilger wrote:
>> The comment that I think needs updating is:
>> 
>> # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
>> # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
>> 
>> The "md5" option no longer works, as discussed in other threads.
> 
> Uh, I think that "md5" still works just fine.

Yes, sorry for the noise.

I was under the impression that md5 was removed for this release, per other
threads that I must not have followed closely enough.


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

[HACKERS] md5 still listed as an option in pg_hba.conf.sample

[Mark Dilger]

The comment that I think needs updating is:

# METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
# "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".

The "md5" option no longer works, as discussed in other threads.

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] Renaming PG_GETARG functions (was Re: PG_GETARG_GISTENTRY?)

[Mark Dilger]

> On Sep 12, 2017, at 1:07 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> [ changing subject line to possibly draw more attention ]
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>>> On Apr 5, 2017, at 9:23 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> In short, if you are supposed to write
>>> FOO  *val = PG_GETARG_FOO(n);
>>> then the macro designer blew it, because the name implies that it
>>> returns FOO, not pointer to FOO.  This should be
>>> FOO  *val = PG_GETARG_FOO_P(n);
> 
>> I have written a patch to fix these macro definitions across src/ and
>> contrib/.
> 

Thanks, Tom, for reviewing my patch.




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

Re: [HACKERS] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Aug 10, 2017, at 11:20 AM, Robert Haas <robertmh...@gmail.com> wrote:
> 
> On Wed, Jul 5, 2017 at 12:14 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> I can understand this, but wonder if I could use something like
>> 
>> FOR I TOTALLY PROMISE TO USE ALL ROWS rec IN EXECUTE sql LOOP
>> ...
>> END LOOP;
> 
> Actually, what you'd need is:
> 
> FOR I TOTALLY PROMISE TO USE ALL ROWS AND IT IS OK TO BUFFER THEM ALL
> IN MEMORY INSTEAD OF FETCHING THEM ONE AT A TIME FROM THE QUERY rec IN
> EXECUTE sql LOOP
> 
> Similarly, RETURN QUERY could be made to work with parallelism if we
> had RETURN QUERY AND IT IS OK TO BUFFER ALL THE ROWS IN MEMORY TWICE
> INSTEAD OF ONCE.
> 
> I've thought a bit about trying to make parallel query support partial
> execution, but it seems wicked hard.  The problem is that you can't
> let the main process do anything parallel-unsafe (e.g., currently,
> write any data) while the there are workers in existence, or things
> may blow up badly.  You could think about fixing that problem by
> having all of the workers exit cleanly when the query is suspended,
> and then firing up new ones when the query is resumed.  However, that
> presents two further problems: (1) having the workers exit cleanly
> when the query is suspended would cause wrong answers unless any
> tuples that the worker has implicitly claimed e.g. by taking a page
> from a parallel scan and returning only some of the tuples on it were
> somehow accounted for and (2) starting and stopping workers over and
> over would be bad for performance.  The second problem could be solved
> by having a persistent pool of workers that attach and detach instead
> of firing up new ones all the time, but that has a host of problems
> all of its own.  The first one would be desirable change for a bunch
> of reasons but is not easy for reasons that are a little longer than I
> feel like explaining right now.

That misses the point I was making.  I was suggesting a syntax where
the caller promises to use all rows without stopping short, and the
database performance problems of having a bunch of parallel workers
suspended in mid query is simply the caller's problem if the caller does
not honor the contract.  Maybe the ability to execute such queries would
be limited to users who are granted a privilege for doing so, and the DBA
can decide not to go around granting that privilege to anybody.
Certainly if this is being used from within a stored procedure, the DBA
can make certain only to use it in cases where there is no execution
path exiting the loop before completion, either because everything is
wrapped up with try/catch syntax and/or because the underlying query
does not call anything that might throw exceptions.

I'm not advocating that currently, as you responded to a somewhat old
email, so really I'm just making clear what I intended at the time.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 19, 2017, at 9:53 AM, Robert Haas  wrote:
> 
> On Mon, Jul 17, 2017 at 6:12 PM, Tom Lane  wrote:
>> So, thinking about how that would actually work ... the thing to do in
>> order to preserve existing on-disk values is to alternate between signed
>> and unsigned interpretations of abstimes.  That is, right now, abstime
>> is signed with origin 1970.  The conversion I'm arguing we should make
>> real soon now is to unsigned with origin 1970.  If the project lives
>> so long, in circa 70 years we'd switch it to signed with origin 2106.
>> Yadda yadda.
> 
> Doesn't this plan amount to breaking pg_upgrade compatibility and
> hoping that nobody notice?  Existing values will be silently
> reinterpreted according to the new rules.  If we had two actually
> separate types and let people convert columns from the old type to the
> new type with just a validation scan, that would be engineering at the
> level of quality that I've come to associate with this project.  

This is what I have done in my fork.  I repurposed the type as "secondstamp"
since I think of it as a timestamp down to second precision (truncating
fractional seconds.)  I changed the Oids assigned to the catalog entries
associated with the type, and considered myself somewhat immune to
the project dropping the abstime type, which the documentation warned
would happen eventually.


> If
> this type is so marginal that we don't want to do that kind of work,
> then I think we should just rip it out; that doesn't preclude someone
> maintaining it in their own fork, or even adding it back as a new type
> in a contrib module with a #define for the base year.  Silently
> changing the interpretation of the same data in the core code, though,
> seems both far too clever and not clever enough.

I would be happy to contribute the "secondstamp" type as part of a patch
that removes the abstime type.  I can add a catalog table that holds the
epoch, and add documentation for the type stating that every time the
epoch changes, their data will be automatically reinterpreted, and as such
they should only use the datatype if that is ok. 

Under this plan, users with abstime columns who upgrade to postgres 11
will not have an easy time, because the type will be removed.  But that is
the same and no worse than what they would get if we just remove the
abstime type in postgres 11 without any replacement. 

I'm not implementing any of this yet, as I expect further objections.

Thoughts?

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 19, 2017, at 10:23 AM, Robert Haas  wrote:
> 
> On Wed, Jul 19, 2017 at 1:12 PM, Tom Lane  wrote:
>>> Doesn't this plan amount to breaking pg_upgrade compatibility and
>>> hoping that nobody notice?
>> 
>> Well, what we'd need to do is document that the type is only meant to be
>> used to store dates within say +/- 30 years from current time.  As long
>> as people adhere to that use-case, the proposal would work conveniently
>> long into the future ...
> 
> Typically, when you try to store an out-of-range value in PostgreSQL,
> you get an ERROR, and that's one of the selling points of PostgreSQL.
> PostgreSQL users regularly beat up other projects for, say, allowing
> -00-00 to be considered a valid date, or any similar perceived
> laxity in enforcing data consistency.  I don't like the idea that we
> can just deviate from that principle whenever adhering to it is too
> much work.

I don't see the relevance of this statement.  I am not planning to allow
abstime data that is outside the range of the new epoch.  Attempts to
cast strings like '1962-07-07 01:02:03' to abstime would draw an
exception with a suitably informative message.

Now, the objection to having on-disk data automagically change meaning
is concerning, and I was particularly unsatisfied with the idea that
NOSTART_ABSTIME and NOEND_ABSTIME would suddenly be
reinterpreted as seconds in the year 2068, which is why I made mention
of it upthread.  I was less concerned with dates prior to 1970 being
reinterpreted, though it is hard to justify why that bothers me less.

>> I'd definitely be on board with just dropping the type altogether despite
>> Mark's concern.
> 
> Then I vote for that option.

I was somewhat surprised when Tom was onboard with the idea of keeping
abstime around (for my benefit).  My original post was in response to his
statement that, right offhand, he couldn't think of any use for abstime that
wasn't handled better by timestamptz (paraphrase), and so I said that
improving storage efficiency was such a use.  I maintain that position.  The
abstime type is a good and useful type, and we will lose that use when we
discard it.  Those who feel otherwise might like to also argue for dropping
float4 because float8 does all the same stuff better.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 18, 2017, at 9:13 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
>> 
>> On Jul 17, 2017, at 2:34 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> 
>>> 
>>> On Jul 17, 2017, at 2:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> 
>>> Mark Dilger <hornschnor...@gmail.com> writes:
>>>>> On Jul 17, 2017, at 12:54 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>>> On Jul 15, 2017, at 3:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>>>> The types abstime, reltime, and tinterval need to go away, or be
>>>>>> reimplemented, sometime well before 2038 when they will overflow.
>>> 
>>>>> These types provide a 4-byte datatype for storing real-world second
>>>>> precision timestamps, as occur in many log files.  Forcing people to
>>>>> switch to timestamp or timestamptz will incur a 4 byte per row
>>>>> penalty.  In my own builds, I have changed the epoch on these so
>>>>> they won't wrap until sometime after 2100 C.E.  I see little point in
>>>>> switching to an 8-byte millisecond precision datatype when a perfectly
>>>>> good 4-byte second precision datatype already serves the purpose.
>>> 
>>> Well, if you or somebody is willing to do the legwork, I'd be on board
>>> with a plan that says that every 68 years we redefine the origin of
>>> abstime.  I imagine it could be done so that currently-stored abstime
>>> values retain their present meaning as long as they're not too old.
>>> For example the initial change would toss abstimes before 1970 overboard,
>>> repurposing that range of values as being 2038-2106, but values between
>>> 1970 and 2038 still mean the same as they do today.  If anybody still
>>> cares in circa 2085, we toss 1970-2038 overboard and move the origin
>>> again, lather rinse repeat.
>>> 
>>> But we're already past the point where it would be time to make the
>>> first such switch, if we're gonna do it.  So I'd like to see somebody
>>> step up to the plate sooner not later.
>> 
>> Assuming other members of the community would not object to such
>> a plan, I'd be willing to step up to that plate.  I'll wait a respectable 
>> time,
>> maybe until tomorrow, to allow others to speak up.
> 
> There was not much conversation about this, so I went ahead with what
> I think makes a logical first step.  The attached patch removes the tinterval
> datatype from the sources.
> 
> I intend to remove reltime next, and then make the changes to abstime in
> a third patch.

As predicted, this second patch (which should be applied *after* the prior
tinterval_abatement patch) removes the reltime datatype from the sources.

mark



reltime_abatement.patch.1
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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 2:34 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
>> 
>> On Jul 17, 2017, at 2:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> Mark Dilger <hornschnor...@gmail.com> writes:
>>>> On Jul 17, 2017, at 12:54 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>> On Jul 15, 2017, at 3:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>>> The types abstime, reltime, and tinterval need to go away, or be
>>>>> reimplemented, sometime well before 2038 when they will overflow.
>> 
>>>> These types provide a 4-byte datatype for storing real-world second
>>>> precision timestamps, as occur in many log files.  Forcing people to
>>>> switch to timestamp or timestamptz will incur a 4 byte per row
>>>> penalty.  In my own builds, I have changed the epoch on these so
>>>> they won't wrap until sometime after 2100 C.E.  I see little point in
>>>> switching to an 8-byte millisecond precision datatype when a perfectly
>>>> good 4-byte second precision datatype already serves the purpose.
>> 
>> Well, if you or somebody is willing to do the legwork, I'd be on board
>> with a plan that says that every 68 years we redefine the origin of
>> abstime.  I imagine it could be done so that currently-stored abstime
>> values retain their present meaning as long as they're not too old.
>> For example the initial change would toss abstimes before 1970 overboard,
>> repurposing that range of values as being 2038-2106, but values between
>> 1970 and 2038 still mean the same as they do today.  If anybody still
>> cares in circa 2085, we toss 1970-2038 overboard and move the origin
>> again, lather rinse repeat.
>> 
>> But we're already past the point where it would be time to make the
>> first such switch, if we're gonna do it.  So I'd like to see somebody
>> step up to the plate sooner not later.
> 
> Assuming other members of the community would not object to such
> a plan, I'd be willing to step up to that plate.  I'll wait a respectable 
> time,
> maybe until tomorrow, to allow others to speak up.

There was not much conversation about this, so I went ahead with what
I think makes a logical first step.  The attached patch removes the tinterval
datatype from the sources.

I intend to remove reltime next, and then make the changes to abstime in
a third patch.

mark



tinterval_abatement.patch.1
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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 3:12 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>>> On Jul 17, 2017, at 2:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Well, if you or somebody is willing to do the legwork, I'd be on board
>>> with a plan that says that every 68 years we redefine the origin of
>>> abstime.  I imagine it could be done so that currently-stored abstime
>>> values retain their present meaning as long as they're not too old.
>>> For example the initial change would toss abstimes before 1970 overboard,
>>> repurposing that range of values as being 2038-2106, but values between
>>> 1970 and 2038 still mean the same as they do today.  If anybody still
>>> cares in circa 2085, we toss 1970-2038 overboard and move the origin
>>> again, lather rinse repeat.
> 
>> Assuming other members of the community would not object to such
>> a plan, I'd be willing to step up to that plate.
> 
> So, thinking about how that would actually work ... the thing to do in
> order to preserve existing on-disk values is to alternate between signed
> and unsigned interpretations of abstimes.  That is, right now, abstime
> is signed with origin 1970.  The conversion I'm arguing we should make
> real soon now is to unsigned with origin 1970.  If the project lives
> so long, in circa 70 years we'd switch it to signed with origin 2106.
> Yadda yadda.
> 
> Now, this should mostly work conveniently, except that we have
> +/-infinity (NOEND_ABSTIME/NOSTART_ABSTIME) to deal with.  Those
> are nicely positioned at the ends of the signed range so that
> abstime_cmp_internal() doesn't need to treat them specially.
> In an unsigned world they'd be smack in the middle of the range.
> We could certainly teach abstime_cmp_internal to special-case them
> and deliver logically correct results, but there's a bigger problem:
> those will correspond to two seconds in January 2038 that will need
> to be representable when the time comes.  Maybe we can make that
> work by defining the values past 2038 as being two seconds less than
> you might otherwise expect, but I bet it's gonna be messy.  It might
> be saner to just desupport +/-infinity for abstime.
> 
> The same goes for INVALID_ABSTIME, which doesn't have any clear
> use-case that I know of, and is certainly not documented.

I use the abstime type in some catalog tables, and if I allowed those
fields to have something equivalent to NULL, which I do not, I would need
INVALID_ABSTIME, since NULL is not allowed in the constant header of a
catalog table.

I wonder if old versions of postgres had catalog tables with abstime used
in this way?  Other than that, I can't think of a reason to use INVALID_ABSTIME.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 3:56 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>> On Jul 17, 2017, at 3:12 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Now, this should mostly work conveniently, except that we have
>>> +/-infinity (NOEND_ABSTIME/NOSTART_ABSTIME) to deal with ... It might
>>> be saner to just desupport +/-infinity for abstime.
> 
>> I don't use those values, so it is no matter to me if we desupport them.  It
>> seems a bit pointless, though, because we still have to handle legacy
>> values that we encounter.  I assume some folks will have those values in
>> their tables when they upgrade.
> 
> Well, some folks will also have pre-1970 dates in their tables, no?
> We're just blowing those off.  They'll print out as some post-2038
> date or other, and too bad.
> 
> Basically, the direction this is going in is that abstime will become
> an officially supported type, but its range of supported values is "not
> too many decades either way from now".  If you are using it to store
> very old dates then You're Doing It Wrong, and eventually you'll get
> bitten.  Given that contract, I don't see a place for +/-infinity.

Works for me.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 3:12 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>>> On Jul 17, 2017, at 2:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Well, if you or somebody is willing to do the legwork, I'd be on board
>>> with a plan that says that every 68 years we redefine the origin of
>>> abstime.  I imagine it could be done so that currently-stored abstime
>>> values retain their present meaning as long as they're not too old.
>>> For example the initial change would toss abstimes before 1970 overboard,
>>> repurposing that range of values as being 2038-2106, but values between
>>> 1970 and 2038 still mean the same as they do today.  If anybody still
>>> cares in circa 2085, we toss 1970-2038 overboard and move the origin
>>> again, lather rinse repeat.
> 
>> Assuming other members of the community would not object to such
>> a plan, I'd be willing to step up to that plate.
> 
> So, thinking about how that would actually work ... the thing to do in
> order to preserve existing on-disk values is to alternate between signed
> and unsigned interpretations of abstimes.  That is, right now, abstime
> is signed with origin 1970.  The conversion I'm arguing we should make
> real soon now is to unsigned with origin 1970.  If the project lives
> so long, in circa 70 years we'd switch it to signed with origin 2106.
> Yadda yadda.

Right, I already had the same idea.  That's not how I am doing it in my
fork currently, but that's what you would want to do to support any values
already stored somewhere.

> Now, this should mostly work conveniently, except that we have
> +/-infinity (NOEND_ABSTIME/NOSTART_ABSTIME) to deal with.  Those
> are nicely positioned at the ends of the signed range so that
> abstime_cmp_internal() doesn't need to treat them specially.
> In an unsigned world they'd be smack in the middle of the range.
> We could certainly teach abstime_cmp_internal to special-case them
> and deliver logically correct results, but there's a bigger problem:
> those will correspond to two seconds in January 2038 that will need
> to be representable when the time comes.  Maybe we can make that
> work by defining the values past 2038 as being two seconds less than
> you might otherwise expect, but I bet it's gonna be messy.  It might
> be saner to just desupport +/-infinity for abstime.

I don't use those values, so it is no matter to me if we desupport them.  It
seems a bit pointless, though, because we still have to handle legacy
values that we encounter.  I assume some folks will have those values in
their tables when they upgrade.

> The same goes for INVALID_ABSTIME, which doesn't have any clear
> use-case that I know of, and is certainly not documented.

Likewise, I don't know how to desupport this while still supporting legacy
tables.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 2:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>>> On Jul 17, 2017, at 12:54 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> On Jul 15, 2017, at 3:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>> The types abstime, reltime, and tinterval need to go away, or be
>>>> reimplemented, sometime well before 2038 when they will overflow.
> 
>>> These types provide a 4-byte datatype for storing real-world second
>>> precision timestamps, as occur in many log files.  Forcing people to
>>> switch to timestamp or timestamptz will incur a 4 byte per row
>>> penalty.  In my own builds, I have changed the epoch on these so
>>> they won't wrap until sometime after 2100 C.E.  I see little point in
>>> switching to an 8-byte millisecond precision datatype when a perfectly
>>> good 4-byte second precision datatype already serves the purpose.
> 
> Well, if you or somebody is willing to do the legwork, I'd be on board
> with a plan that says that every 68 years we redefine the origin of
> abstime.  I imagine it could be done so that currently-stored abstime
> values retain their present meaning as long as they're not too old.
> For example the initial change would toss abstimes before 1970 overboard,
> repurposing that range of values as being 2038-2106, but values between
> 1970 and 2038 still mean the same as they do today.  If anybody still
> cares in circa 2085, we toss 1970-2038 overboard and move the origin
> again, lather rinse repeat.
> 
> But we're already past the point where it would be time to make the
> first such switch, if we're gonna do it.  So I'd like to see somebody
> step up to the plate sooner not later.

Assuming other members of the community would not object to such
a plan, I'd be willing to step up to that plate.  I'll wait a respectable time,
maybe until tomorrow, to allow others to speak up.

> I'd be inclined to toss reltime and tinterval overboard in any case.

Yeah, I don't use those either, preferring to cast abstime to timestamp
(or timestamptz) prior to doing any math on them.

> 
>> Oh, and if you could be so kind, please remove them in a commit that
>> does nothing else.  It's much easier to skip the commit that way.
> 
> We doubtless would do that, but you're fooling yourself if you imagine
> that you can maintain such a fork for long while still tracking the
> community version otherwise.  Once those hand-assigned OIDs are free
> they'll soon be absorbed by future feature patches, and then you'll
> have enormous merge problems.

Oh, not so much.  Knowing that they were going to be deprecated, I
already moved the Oids for these to something higher than 1.  In
my own builds, the Oid generator does not assign Oids lower than 65536,
which leaves room for me to assign in the 1-65535 range without
merge headaches.  It would, however, be simpler if the types did not
go away, as that would cause minor merge headaches elsewhere, such
as in the regression tests.

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 17, 2017, at 12:54 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Jul 15, 2017, at 3:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> The types abstime, reltime, and tinterval need to go away, or be
>> reimplemented, sometime well before 2038 when they will overflow.
>> It's not too soon to start having a plan for that, especially seeing
>> that it seems to take a decade or more for us to actually get rid
>> of anything we've deprecated.
>> 
>> Right offhand, I don't think there is any functionality in these
>> types that isn't handled as well or better by timestamptz, interval,
>> and tstzrange respectively.  
> 
> These types provide a 4-byte datatype for storing real-world second
> precision timestamps, as occur in many log files.  Forcing people to
> switch to timestamp or timestamptz will incur a 4 byte per row
> penalty.  In my own builds, I have changed the epoch on these so
> they won't wrap until sometime after 2100 C.E.  I see little point in
> switching to an 8-byte millisecond precision datatype when a perfectly
> good 4-byte second precision datatype already serves the purpose.
> 
> That said, I am fully aware that these are deprecated and expect you
> will remove them, at which time I'll have to keep them in my tree
> and politely refuse to merge in your change which removes them.

Oh, and if you could be so kind, please remove them in a commit that
does nothing else.  It's much easier to skip the commit that way.

Thanks!

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] Something for the TODO list: deprecating abstime and friends

[Mark Dilger]

> On Jul 15, 2017, at 3:00 PM, Tom Lane  wrote:
> 
> The types abstime, reltime, and tinterval need to go away, or be
> reimplemented, sometime well before 2038 when they will overflow.
> It's not too soon to start having a plan for that, especially seeing
> that it seems to take a decade or more for us to actually get rid
> of anything we've deprecated.
> 
> Right offhand, I don't think there is any functionality in these
> types that isn't handled as well or better by timestamptz, interval,
> and tstzrange respectively.  

These types provide a 4-byte datatype for storing real-world second
precision timestamps, as occur in many log files.  Forcing people to
switch to timestamp or timestamptz will incur a 4 byte per row
penalty.  In my own builds, I have changed the epoch on these so
they won't wrap until sometime after 2100 C.E.  I see little point in
switching to an 8-byte millisecond precision datatype when a perfectly
good 4-byte second precision datatype already serves the purpose.

That said, I am fully aware that these are deprecated and expect you
will remove them, at which time I'll have to keep them in my tree
and politely refuse to merge in your change which removes them.

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] Revisiting NAMEDATALEN

[Mark Dilger]

> On Jul 7, 2017, at 2:53 AM, Emrul  wrote:
> 
> Tom, thank you for that pointer.  I get now that it is not free and therefore
> why its not something that should be changed by default.
> 
> I guess the problem is 'build your own copy' (i.e. compiling from source) is
> something that sends most DB teams running into the hills.

To make matters worse, if you change NAMEDATALEN, compile, and run
'make check', some of the tests will fail.  The tests are very sensitive to the
exact output of the sql they execute, and changing NAMEDATALEN, or
indeed any one of many other options, causes some of the test output to
change. Even configure's options, such as --with-blocksize, cannot be
changed from the default value without potentially breaking the regression
tests.

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] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Jul 5, 2017, at 5:30 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> 
> On Wed, Jul 5, 2017 at 9:44 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> 
>>> On Jul 3, 2017, at 10:25 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>> 
>>> On Mon, Jul 3, 2017 at 8:57 PM, Simon Riggs <si...@2ndquadrant.com> wrote:
>>>> On 30 June 2017 at 05:14, Amit Kapila <amit.kapil...@gmail.com> wrote:
>>>> 
>>>>> This is explained in section 15.2 [1], refer below para:
>>>>> "The query might be suspended during execution. In any situation in
>>>>> which the system thinks that partial or incremental execution might
>>>>> occur, no parallel plan is generated. For example, a cursor created
>>>>> using DECLARE CURSOR will never use a parallel plan. Similarly, a
>>>>> PL/pgSQL loop of the form FOR x IN query LOOP .. END LOOP will never
>>>>> use a parallel plan, because the parallel query system is unable to
>>>>> verify that the code in the loop is safe to execute while parallel
>>>>> query is active."
>>>> 
>>>> Can you explain "unable to verify that the code in the loop is safe to
>>>> execute while parallel query is active". Surely we aren't pushing code
>>>> in the loop into the actual query, so the safety of command in the FOR
>>>> loop has nothing to do with the parallel safety of the query.
>>>> 
>>>> Please give an example of something that would be unsafe? Is that
>>>> documented anywhere, README etc?
>>>> 
>>>> FOR x IN query LOOP .. END LOOP
>>>> seems like a case that would be just fine, since we're going to loop
>>>> thru every row or break early.
>>>> 
>>> 
>>> It is not fine because we don't support partial execution support.  In
>>> above case, if the loop breaks, we can't break parallel query
>>> execution.  Now, I don't think it will impossible to support the same,
>>> but as of now, parallel subsystem doesn't have such a support.
>> 
>> I can understand this, but wonder if I could use something like
>> 
>> FOR I TOTALLY PROMISE TO USE ALL ROWS rec IN EXECUTE sql LOOP
>> ...
>> END LOOP;
>> 
>> if I hacked the grammar up a bit.  Would the problem go away, or would
>> I still have problems when exceptions beyond my control get thrown inside
>> the loop?
>> 
> 
> I don't think it is just a matter of hacking grammar, internally we
> are using cursor fetch to fetch the rows and there we are passing some
> fixed number of rows to fetch which again is a killer to invoke the
> parallel query.

Is the risk that a RETURN will be executed within the loop block the only
risk that is causing parallel query to be disabled in these plpgsql loops?
If so, it seems that a plpgsql language syntax which created a loop that
disabled RETURN from within the loop body would solve the problem.  I
do not propose any such language extension.  It was just a thought
experiment.

Is there a conditional somewhere in the source that says, in effect, "if this
is a plpgsql loop, then disable parallel query"?  If so, and if I removed that
check such that parallel query would run in plpgsql loops, would I only
get breakage when I returned out of a loop?  Or would there be other
situations where that would break?

Thanks,

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] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Jul 3, 2017, at 10:25 PM, Amit Kapila  wrote:
> 
> On Mon, Jul 3, 2017 at 8:57 PM, Simon Riggs  wrote:
>> On 30 June 2017 at 05:14, Amit Kapila  wrote:
>> 
>>> This is explained in section 15.2 [1], refer below para:
>>> "The query might be suspended during execution. In any situation in
>>> which the system thinks that partial or incremental execution might
>>> occur, no parallel plan is generated. For example, a cursor created
>>> using DECLARE CURSOR will never use a parallel plan. Similarly, a
>>> PL/pgSQL loop of the form FOR x IN query LOOP .. END LOOP will never
>>> use a parallel plan, because the parallel query system is unable to
>>> verify that the code in the loop is safe to execute while parallel
>>> query is active."
>> 
>> Can you explain "unable to verify that the code in the loop is safe to
>> execute while parallel query is active". Surely we aren't pushing code
>> in the loop into the actual query, so the safety of command in the FOR
>> loop has nothing to do with the parallel safety of the query.
>> 
>> Please give an example of something that would be unsafe? Is that
>> documented anywhere, README etc?
>> 
>> FOR x IN query LOOP .. END LOOP
>> seems like a case that would be just fine, since we're going to loop
>> thru every row or break early.
>> 
> 
> It is not fine because we don't support partial execution support.  In
> above case, if the loop breaks, we can't break parallel query
> execution.  Now, I don't think it will impossible to support the same,
> but as of now, parallel subsystem doesn't have such a support.

I can understand this, but wonder if I could use something like

FOR I TOTALLY PROMISE TO USE ALL ROWS rec IN EXECUTE sql LOOP
...
END LOOP;

if I hacked the grammar up a bit.  Would the problem go away, or would
I still have problems when exceptions beyond my control get thrown inside
the loop?  And if exception handling is a problem in the loop, are exceptions
somehow not a problem in other parallel queries?

Obviously I mean that syntax above in jest, but the question is in ernest.

Thanks,

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] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Jun 29, 2017, at 8:55 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
> Changing myfunc to create a temporary table, to execute the sql to populate
> that temporary table, and to then loop through the temporary table's rows
> fixes the problem.  For the real-world example where I hit this, that single
> change decreases the runtime from 13.5 seconds to 2.5 seconds.

Actually, this is wrong.  On further review, by the time I had changed the
function definition, the data in the test server I was querying had likely 
changed
enough for the performance to change.  That duped me into thinking I had
found a work-around.  I can no longer reproduce any performance improvement
in this manner.

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] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

> On Jun 29, 2017, at 9:14 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> 
> On Fri, Jun 30, 2017 at 9:25 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> Hackers,
>> 
>> In src/pl/plpgsql/src/pl_exec.c: exec_run_select intentionally does not
>> allow a parallel plan if a portal will be returned.  This has the practical
>> consequence that a common coding practice (at least for me) of doing
>> something like:
>> 
>> create function myfunc(arg1 text, arg2 text) returns setof myfunctype as $$
>> declare
>>sql text;
>>result  myfunctype;
>> begin
>>-- unsafe interpolation, but this is just a code example
>>sql := 'select foo from bar where a = ' || arg1 || ' and b = ' || 
>> arg2;
>>for result in execute sql loop
>>return next result;
>>end loop;
>>return;
>> end;
>> $$ language plpgsql volatile;
>> 
>> can't run the generated 'sql' in parallel.  I think this is understandable, 
>> but
>> the documentation of this limitation in the sgml docs is thin.  Perhaps
>> someone who understands this limitation better than I do can document it?
>> 
> 
> This is explained in section 15.2 [1], refer below para:
> "The query might be suspended during execution. In any situation in
> which the system thinks that partial or incremental execution might
> occur, no parallel plan is generated. For example, a cursor created
> using DECLARE CURSOR will never use a parallel plan. Similarly, a
> PL/pgSQL loop of the form FOR x IN query LOOP .. END LOOP will never
> use a parallel plan, because the parallel query system is unable to
> verify that the code in the loop is safe to execute while parallel
> query is active."
> 
> Check if that clarifies your doubts, otherwise, we might need to write
> something more so that it can be easier for users to understand.

That's what I was looking for.  I think I had trouble finding it since I was
using plpgsql (notice no slash) and "parallel worker" and so forth to try
to find it.  "PL/pgSQL" and "parallel query" are not terms I use, and did
not notice them buried a few sentences down in this paragraph.

Thanks for the citation.

mark

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

[HACKERS] Request more documentation for incompatibility of parallelism and plpgsql exec_run_select

[Mark Dilger]

Hackers,

In src/pl/plpgsql/src/pl_exec.c: exec_run_select intentionally does not
allow a parallel plan if a portal will be returned.  This has the practical
consequence that a common coding practice (at least for me) of doing
something like:

create function myfunc(arg1 text, arg2 text) returns setof myfunctype as $$
declare
sql text;
result  myfunctype; 
begin
-- unsafe interpolation, but this is just a code example
sql := 'select foo from bar where a = ' || arg1 || ' and b = ' || arg2;
for result in execute sql loop
return next result;
end loop;
return;
end;
$$ language plpgsql volatile;

can't run the generated 'sql' in parallel.  I think this is understandable, but
the documentation of this limitation in the sgml docs is thin.  Perhaps
someone who understands this limitation better than I do can document it?
Changing myfunc to create a temporary table, to execute the sql to populate
that temporary table, and to then loop through the temporary table's rows
fixes the problem.  For the real-world example where I hit this, that single
change decreases the runtime from 13.5 seconds to 2.5 seconds.  This
makes sense to me, because doing it that way means pl_exec knows that
it won't be returning a portal for the executed sql, so the parallel plan is
still allowed.

Sorry to be a nag.  I'm only trying to help the next person who might
otherwise trip over this limitation.  Perhaps this belongs in section 15.3.4
or 42.5.4.

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] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

[Mark Dilger]

> On Jun 4, 2017, at 2:19 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> On 2017-06-04 14:16:14 -0700, Mark Dilger wrote:
>> Sorry, I was not clear.  What I meant to get at was that if you remove from 
>> the
>> executor all support for SRFs inside case statements, you might foreclose 
>> the option
>> of extending the syntax at some later date to allow aggregates over
>> SRFs.
> 
> Seems very unlikely that we'd ever want to do that.  The right way to do
> this is to simply move the SRF into the from list.  Having the executor
> support arbitrary sources of tuples would just complicate and slow down
> already complicated and slow code...
> 
> 
>> I'm
>> not saying that this works currently, but in principle if you allowed that 
>> SUM() that
>> I put up there, you'd get back exactly one row from it, same as you get from 
>> the
>> ELSE clause.  That would seem to solve the problem without going so far as
>> completely disallowing the SRF altogether.
> 
> But what would the benefit be?

In my example, the aggregate function is taking a column from the table as
an argument, so the output of the aggregate function needs to be computed per 
row,
not just once.  And if the function is expensive, or has side-effects, you might
only want it to execute for those rows where the CASE statement is true, rather
than for all of them.  You may get that same behavior using lateral or some 
such,
I'm uncertain, but in a complicated CASE statement, it be more straightforward
to write something like:

SELECT
CASE
WHEN t.x = 'foo' THEN expensive_aggfunc1(srf1(t.y,t.z))
WHEN t.x = 'bar' THEN expensive_aggfunc2(srf2(t.y,t.z))
WHEN t.x = 'baz' THEN expensive_aggfunc3(srf3(t.y,t.z))

WHEN t.x = 'zzz' THEN expensive_aggfuncN(srfN(t.y,t.z))
ELSE 5
END
FROM mytable t;

Than to try to write it any other way.


I'm not advocating anything here, even though it may sound that way to you.
I'm just thinking this thing through, given that you may be committing a removal
of functionality that we want back at some later time.

Out of curiosity, how would you rewrite what I have above such that the
aggregate function is not inside the case statement, and the expensive_aggfuncs
are only called for those (t.y,t.z) that are actually appropriate?


Mark Dilger

-- 
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] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

[Mark Dilger]

> On Jun 4, 2017, at 12:35 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> Hi Mark,
> 
> On 2017-06-04 11:55:03 -0700, Mark Dilger wrote:
>>> Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
>>> SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
>>> sure a reasonable behaviour even exists.  IIRC I'd argued in the
>>> original SRF thread that we should just throw an error, and I think we'd
>>> concluded that we'd not do so for now.
>> 
>> I am trying to get my head around the type of query you and Tom
>> are discussing.  When you say you are unsure a reasonable behavior
>> even exists, are you saying such queries have no intuitive meaning?
> 
> I'm not saying that there aren't some cases where it's intuitive, but
> there's definitely lots that don't have intuitive meaning.  Especially
> when there are multiple SRFs in the same targetlist.
> 
> Do I understand correctly that you're essentially advocating for the <
> v10 behaviour?

No, I'm not advocating either way.  I merely wanted to know which queries
you and Tom were talking about.

>  It'd be nontrivial to implement that, without loosing
> the significant benefits (Significantly slower, higher code complexity,
> weird behaviour around multiple SRFs) gained by removing the previous
> implementation.   I'd really like to see some examples of when all of
> this is useful - I've yet to see a realistic one that's not just as
> easily written differently
> 
> 
>> Can you give an example of such a query which has no intuitive
>> meaning?  Perhaps I am not thinking about the right kind of queries.
>> I have been thinking about examples like:
>> 
>> SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
>>  FROM table_with_columns_x_and_y_and_z;
>> 
>> Which to me gives 'z' output rows per table row where y is true, and
>> one output row per table row where y is false.
> 
> Try any query that has one SRF outside of the CASE, and one inside.  In
> the old behaviour that'll make the total number of rows returned nearly
> undeterministic because of the least-common-multiple behaviour.
> 
>> That could be changed with an aggregate function such as:
>> SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
>>  FROM table_with_columns_x_and_y;
> 
> That query doesn't work.  First off, aggregates don't take set arguments
> (neither in old nor new releases), secondly aggregates are evaluated
> independently of CASE/COALESCE statements, thirdly you're missing group
> bys.  Those all are independent of the v10 changes.

Sorry, I was not clear.  What I meant to get at was that if you remove from the
executor all support for SRFs inside case statements, you might foreclose the 
option
of extending the syntax at some later date to allow aggregates over SRFs.  I'm
not saying that this works currently, but in principle if you allowed that 
SUM() that
I put up there, you'd get back exactly one row from it, same as you get from the
ELSE clause.  That would seem to solve the problem without going so far as
completely disallowing the SRF altogether.  The reasons for not putting a GROUP 
BY
clause in the example are (a) I don't know where it would go, syntactically, 
and (b)
it would defeat the purpose, because once you are grouping, you again have the
possibility of getting more than one row.

I'm not advocating implementing this; I'm just speculating about possible future
features in which SRFs inside a case statement might be allowed.

> 
>> Thanks, and my apologies if I am merely lacking sufficient imagination to
>> think of a proper example.
> 
> Might be worthwhile to reread the thread about the SRF reimplementation.
> 
> https://www.postgresql.org/message-id/20160822214023.aaxz5l4igypow...@alap3.anarazel.de

This helps, thanks!

Mark Dilger



-- 
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] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

[Mark Dilger]

> On Jun 4, 2017, at 11:55 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
>   FROM table_with_columns_x_and_y;

Sorry, this table is supposed to be the same as the previous one,

table_with_columns_x_and_y_and_z



-- 
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] PostgreSQL 10 changes in exclusion constraints - did something change? CASE WHEN behavior oddity

[Mark Dilger]

> On Jun 2, 2017, at 8:11 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> Hi,
> 
> 
> On 2017-06-02 22:53:00 -0400, Tom Lane wrote:
>> I think you've got enough on your plate.  I can take care of whatever
>> we decide to do here.
> 
> Thanks.
> 
> 
>> Andres Freund <and...@anarazel.de> writes:
>>>> Another possibility is to say that we've broken this situation
>>>> irretrievably and we should start throwing errors for SRFs in
>>>> places where they'd be conditionally evaluated.  That's not real
>>>> nice perhaps, but it's better than the way things are right now.
>> 
>>> I'd be ok with that too, but I don't really see a strong need so far.
>> 
>> The argument for this way is basically that it's better to break
>> apps visibly than silently.
> 
> Right, I got that.
> 
> 
>> The behavior for SRF-inside-CASE is
>> not going to be the same as before even if we implement the fix
>> I suggest above, and it's arguable that this new behavior is not
>> at all intuitive.
> 
> Yea, I'm not a big fan of the either the pre v10 or the v10 behaviour of
> SRFs inside coalesce/case.  Neither is really resonable imo - I'm not
> sure a reasonable behaviour even exists.  IIRC I'd argued in the
> original SRF thread that we should just throw an error, and I think we'd
> concluded that we'd not do so for now.

I am trying to get my head around the type of query you and Tom
are discussing.  When you say you are unsure a reasonable behavior
even exists, are you saying such queries have no intuitive meaning?

Can you give an example of such a query which has no intuitive
meaning?  Perhaps I am not thinking about the right kind of queries.
I have been thinking about examples like:

SELECT x, CASE WHEN y THEN generate_series(1,z) ELSE 5 END
FROM table_with_columns_x_and_y_and_z;

Which to me gives 'z' output rows per table row where y is true, and
one output row per table row where y is false.  That could be changed
with an aggregate function such as:

SELECT x, CASE WHEN y THEN SUM(generate_series(1,z)) ELSE 5 END
FROM table_with_columns_x_and_y;

Which to me gives one output row per table row regardless of whether y
is true.


Thanks, and my apologies if I am merely lacking sufficient imagination to
think of a proper example.

Mark Dilger


-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On Jun 2, 2017, at 9:59 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Robert Haas <robertmh...@gmail.com> writes:
>> Also, you're attacking a straw man. Accidentally storing a meaningless
>> parse location in the catalog isn't a feature, and we shouldn't
>> pretend otherwise.
> 
> It's not meaningless, and it is a feature, useful for debugging.
> Otherwise you'd have a hard time telling one occurrence of e.g. "+" from
> another when you're trying to make sense of a stored tree.  We worked out
> all this behavior ages ago for other expression node trees that are stored
> in the catalogs (default expressions, index expressions, check
> constraints, etc etc) and I don't see a reason for partition expressions
> to be different.

Ok, that makes more sense now.  Thanks for clarifying the history of how
and why the location field from parsing is getting stored.

> I agree that this means you can't just strcmp a couple of stored node tree
> strings to decide if they're equal, but you couldn't anyway --- a moment's
> perusal of equalfuncs.c will show you other special cases, each with their
> own rationale.  Maybe you'd like to complain that every one of those
> rationales is wrongheaded but I do not think you will get far.
> 
> I think the best advice for Mark is to look at pg_get_expr() output and
> see if that matches.

Yes, I'm already doing that based on the discussion so far.

Mark Dilger


-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On Jun 1, 2017, at 6:31 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>> When you guys commit changes that impact partitioning, I notice, and change
>> my code to match.  But in this case, it seemed to me the change that got
>> committed was not thought through, and it might benefit the community for
>> me to point it out, rather than quietly make my code behave the same as
>> what got committed.
> 
> Let me explain the project standards in words of one syllable: user code
> should not examine the contents of node trees.  That's what pg_get_expr
> is for.  There is not, never has been, and never will be any guarantee
> that we won't whack those structures around in completely arbitrary ways,
> as long as we do a catversion bump along with it.

I have already dealt with this peculiarity and don't care much at this point, 
but
I think your characterization of my position is inaccurate:

I'm really not talking about examining the contents of node trees.  I'm only
talking about comparing two of them for equality, and getting false as an 
answer,
when they are actually equal.  Asking "Is A == B" is not "examining the 
contents",
it is asking the project supplied comparator function to examine the contents,
which is on par with asking the project supplied pg_get_expr function to do so.
There is currently only one production in gram.y in the public sources that
creates partitions, so you don't notice the relpartbound being dependent on
which grammar production defined the table.  I notice, and think it is an odd
thing to encode in the the relpartbound field.  Bumping the catversion is 
irrelevant
if you have two or more different productions in gram.y that give rise to these
field values.  Apparently, you don't care.  Fine by me.  It just seemed to me an
oversight when I first encountered it, rather than something anybody would
intentionally commit to the sources.

Mark Dilger



-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 6:32 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> 
> wrote:
> 
> On 2017/06/01 4:50, Robert Haas wrote:
>> On Wed, May 31, 2017 at 3:40 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>> recent changes have introduced the :location field to the partboundspec
>>> in pg_catalog.pg_class.  This means that if two identical tables with
>>> identical partitioning scheme are created, but one is done before a change
>>> to gram.y, and the other after a change to gram.y, the relpartbound fields
>>> for those two tables could show up as different.
>>> 
>>> Can we perhaps remove the :location field here?  If not, could somebody
>>> please defend why this belongs in the catalog entries for the table?  Sorry
>>> if I am missing something obvious...
> 
> I now think it's kind of annoying too, although not exactly unprecedented
> as others have pointed out.  As you well might know, the location field in
> the parse node is to position the error cursor at the correct point in the
> erring command text. 

I knew about the location field already, but not that it was already occurring
elsewhere in the catalogs.  I just never paid attention to any of the columns
that are doing so.  Alvaro's criticisms of my complaint were quite informative.
(Thanks, Alvaro.)  I think standardizing the location field to -1 at some point
in all such places would be a good idea, though I am not sufficiently qualified
to say if that should be in 10 or 11, nor whether doing so might cause
backwards compatibility issues, nor whether those would be too much cost
to justify the changes.

> By the way, didn't you first have to come across that?  The commit
> (80f583ffe93) that introduced location field to partboundspec also bumped
> up the catalog version, so you would have to reinitialize the database
> directory anyway.

I don't have my work in production yet.  Each time I merge changes into
my repository, I run "make check-world", and that of course re-initializes
the data directories for the test databases.

Mark Dilger

-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 4:00 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> 
> Mark Dilger wrote:
> 
>>>>
>>>>relpartbound
>>>>  | 
>>>>
>>>> relpartbound   
>>>> | ?column? | ?column?
>>>> ++--+--
>>>> {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
>>>> :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
>>>> false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
>>>> :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l 
>>>> :listdatums ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 
>>>> :constlen 2 :constbyval true :constisnull false :location -1 :constvalue 2 
>>>> [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f
>>>> | f  
>>>> (1 row)
> 
>> I concede that mitigates the problem somewhat, though I still think a user 
>> may look
>> at pg_class, see there is a column that appears to show the partition 
>> boundaries,
>> and then decide to check whether two tables have the same partition 
>> boundaries
>> by comparing those fields, without passing them first through pg_get_expr(), 
>> a
>> function they may never have heard of.
> 
> How do you expect that the used would actually interpret the above
> weird-looking value?  There's no way to figure out what operations are
> being done in that ugly blob of text.

I don't know how the average user would use it.  I can only tell you what
I am doing, which may be on the fringe of what users do typically.

I have modified the system to add a number of catalog tables not normally
present in postgres.  A few of those catalog tables are partitioned.  Since
they have to be set up at bootstrap time, I can't use the CREATE TABLE
syntax in some src/backend/catalog/*.sql file to create them, I have to
create them with header files, genbki.pl and friends.  There is no support
for this, so I created my own.  That puts me at risk of getting out of sync
with the public sources implementation of partitioning.  As such, I have
regression tests that create at run time partitioned tables that have all the
same columns and boundaries as my catalog tables, and I compare the
pg_class entries against each other to make sure there are no inconsistencies.
When you guys commit changes that impact partitioning, I notice, and change
my code to match.  But in this case, it seemed to me the change that got
committed was not thought through, and it might benefit the community for
me to point it out, rather than quietly make my code behave the same as
what got committed.

I doubt too many people will follow in my footsteps here, but other people
may hit this :location peculiarity for other reasons.

Once again, this is just to give you context about why I said anything in the
first place.

> I suspect it would be better to reduce the location field to -1 as
> proposed, though, since the location has no actual meaning once the node
> is stored standalone rather than as part of a larger command.  However
> it's clear that we're not consistent about doing this elsewhere.

I have no opinion about whether this should be done for 10.0, given the
release schedule.  Changing it for 10.0 or 11.0 seems reasonable to me.

Mark Dilger

-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 3:42 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> On 2017-05-31 15:38:58 -0700, Mark Dilger wrote:
>>> Normal users aren't going to make sense of node trees in the first
>>> place.  You should use pg_get_expr for it:
>>> postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class 
>>> WHERE relpartbound IS NOT NULL;
>>> ┌──┐
>>> │ pg_get_expr  │
>>> ├──┤
>>> │ FOR VALUES IN (1, 2) │
>>> └──┘
>>> (1 row)
>> 
>> I concede that mitigates the problem somewhat, though I still think a user 
>> may look
>> at pg_class, see there is a column that appears to show the partition 
>> boundaries,
>> and then decide to check whether two tables have the same partition 
>> boundaries
>> by comparing those fields, without passing them first through pg_get_expr(), 
>> a
>> function they may never have heard of.
>> 
>> To me, it seems odd to immortalize a SQL parsing field in the catalog 
>> definition of
>> the relation, but perhaps that's just my peculiar sensibilities.  If the 
>> community is more
>> on your side, I'm not going to argue it.
> 
> Given that various other node trees stored in the catalog also have
> location fields, about which I recall no complaints, I don't think this
> is a significant issue.  Nor something that I think makes sense to solve
> in isolation, without tackling all stored expressions.

Ok.  Just to clarify, I didn't come up with this problem as part of some general
code review.  I merged the recent changes into my tree, and my regression
tests broke.  That's fine; that's what they are there to do.  Break, and in so 
doing,
alert me to the fact that the project has changed things that will require me to
make modifications.  It just seemed strange to me that I should be changing 
stuff
to try to get the :location field to be equal in my code to the :location field 
in the
public sources, since it seems to serve no purpose.

Mark Dilger

-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 3:17 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> On 2017-05-31 15:06:06 -0700, Mark Dilger wrote:
>> That's cold comfort, given that most users will be looking at the pg_class
>> table and not writing C code that compares Node objects.  I wrote a bit of
>> regression test logic that checks, and sure enough the relpartbound field
>> shows up as unequal:
>>  
>>   relpartbound   
>>  
>> SELECT a.relpartbound, b.relpartbound, a.relpartbound = b.relpartbound, 
>> a.relpartbound::text = b.relpartbound::text
>>FROM pg_class a, pg_class b
>>WHERE a.relname = 'acct_partitioned_1'
>>  AND b.relname = 'acct_partitioned_2';
>>  
>>   relpartbound   
>>   |  
>>   
>> relpartbound   | 
>> ?column? | ?column?
>> ++--+--
>> {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
>> :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull 
>> false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> 
>> :upperdatums <> :location 82} | {PARTITIONBOUNDSPEC :strategy l :listdatums 
>> ({CONST :consttype 23000 :consttypmod -1 :constcollid 0 :constlen 2 
>> :constbyval true :constisnull false :location -1 :constvalue 2 [ 0 0 0 0 0 0 
>> 0 0 ]}) :lowerdatums <> :upperdatums <> :location 73} | f| f  
>> (1 row)
> 
> Normal users aren't going to make sense of node trees in the first
> place.  You should use pg_get_expr for it:
> postgres[3008][1]=# SELECT pg_get_expr(relpartbound, oid) FROM pg_class WHERE 
> relpartbound IS NOT NULL;
> ┌──┐
> │ pg_get_expr  │
> ├──┤
> │ FOR VALUES IN (1, 2) │
> └──┘
> (1 row)

I concede that mitigates the problem somewhat, though I still think a user may 
look
at pg_class, see there is a column that appears to show the partition 
boundaries,
and then decide to check whether two tables have the same partition boundaries
by comparing those fields, without passing them first through pg_get_expr(), a
function they may never have heard of.

To me, it seems odd to immortalize a SQL parsing field in the catalog 
definition of
the relation, but perhaps that's just my peculiar sensibilities.  If the 
community is more
on your side, I'm not going to argue it.

Mark Dilger



-- 
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_class.relpartbound definition overly brittle

[Mark Dilger]

> On May 31, 2017, at 2:49 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> 
> Mark Dilger wrote:
>> Hackers,
>> 
>> recent changes have introduced the :location field to the partboundspec
>> in pg_catalog.pg_class.  This means that if two identical tables with
>> identical partitioning scheme are created, but one is done before a change
>> to gram.y, and the other after a change to gram.y, the relpartbound fields
>> for those two tables could show up as different.
> 
> Actually, if you look at equalfuncs.c, you'll note that locations are
> not really compared:
> 
> /* Compare a parse location field (this is a no-op, per note above) */
> #define COMPARE_LOCATION_FIELD(fldname) \
>   ((void) 0)
> 
> where the referenced note is:
> 
> * NOTE: it is intentional that parse location fields (in nodes that have
> * one) are not compared.  This is because we want, for example, a variable
> * "x" to be considered equal() to another reference to "x" in the query.

That's cold comfort, given that most users will be looking at the pg_class
table and not writing C code that compares Node objects.  I wrote a bit of
regression test logic that checks, and sure enough the relpartbound field
shows up as unequal:

relpartbound

SELECT a.relpartbound, b.relpartbound, a.relpartbound = b.relpartbound, 
a.relpartbound::text = b.relpartbound::text
FROM pg_class a, pg_class b
WHERE a.relname = 'acct_partitioned_1'
  AND b.relname = 'acct_partitioned_2';

relpartbound

|   
 relpartbound   

 | ?column? | ?column?
++--+--
 {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST :consttype 23000 
:consttypmod -1 :constcollid 0 :constlen 2 :constbyval true :constisnull false 
:location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) :lowerdatums <> :upperdatums 
<> :location 82} | {PARTITIONBOUNDSPEC :strategy l :listdatums ({CONST 
:consttype 23000 :consttypmod -1 :constcollid 0 :constlen 2 :constbyval true 
:constisnull false :location -1 :constvalue 2 [ 0 0 0 0 0 0 0 0 ]}) 
:lowerdatums <> :upperdatums <> :location 73} | f| f  
(1 row)





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

[HACKERS] pg_class.relpartbound definition overly brittle

[Mark Dilger]

Hackers,

recent changes have introduced the :location field to the partboundspec
in pg_catalog.pg_class.  This means that if two identical tables with
identical partitioning scheme are created, but one is done before a change
to gram.y, and the other after a change to gram.y, the relpartbound fields
for those two tables could show up as different.

Can we perhaps remove the :location field here?  If not, could somebody
please defend why this belongs in the catalog entries for the table?  Sorry
if I am missing something obvious...

Thanks,

Mark Dilger


-- 
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] Use of non-restart-safe storage by temp_tablespaces

[Mark Dilger]

> On May 31, 2017, at 7:58 AM, Bruce Momjian <br...@momjian.us> wrote:
> 
> On Wed, May 31, 2017 at 07:53:22AM -0700, Mark Dilger wrote:
>>> Just to clarify, the TEMPORARY clause would allow the tablespace to
>>> start up empty, while normal tablespaces can't do that, right?  One big
>>> problem is that we don't have any way to prevent non-temporary
>>> tablespaces from being created on transient storage.  I wonder if we
>>> should document this restriction, but it seems awkward to do.
>> 
>> It depends what you mean by allowing the tablespace to start up empty.
>> It must not be empty once users can run queries, since the catalogs will 
>> still
>> have entries for the tablespace and its dependent objects.  So, what must
>> happen is that during startup the tablespace and its temporary tables and
>> indexes get recreated if they are missing.
> 
> Uh, I thought only the sessions that created the temporary objects could
> see them, and since they are not in WAL and autovacuum can't see them,
> their non-existence in a temporary tablespace would not be a problem.

You are correct.  I was thinking about an extension to allow unlogged
tablespaces on temporary filesystems, but got the words "unlogged" and
"temporary" mixed up in my thinking and in what I wrote.  I should have
written that unlogged tablespaces would only host unlogged tables and
unlogged indexes, such that users are not surprised to find their data
missing.

On reflection, I think both features are worthwhile, and not at all exclusive
of each other, though unlogged tablespaces is probably considerably more
work to implement.

Mark Dilger

-- 
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] Use of non-restart-safe storage by temp_tablespaces

[Mark Dilger]

> On May 31, 2017, at 6:04 AM, Bruce Momjian <br...@momjian.us> wrote:
> 
> On Wed, May 31, 2017 at 07:53:53AM -0400, Robert Haas wrote:
>> On Tue, May 30, 2017 at 6:50 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>>> On May 29, 2017, at 11:53 AM, Bruce Momjian <br...@momjian.us> wrote:
>>>> Right now we don't document that temp_tablespaces can use
>>>> non-restart-safe storage, e.g. /tmp, ramdisks.  Would this be safe?
>>>> Should we document this?
>>> 
>>> The only safe way to do temporary tablespaces that I have found is to extend
>>> the grammar to allow CREATE TEMPORARY TABLESPACE, and then refuse
>>> to allow the creation of any non-temporary table (or index on same) in that
>>> tablespace.  Otherwise, it is too easy to be surprised to discover that your
>>> table contents have gone missing.
>> 
>> I think this would be a sensible approach.
> 
> Just to clarify, the TEMPORARY clause would allow the tablespace to
> start up empty, while normal tablespaces can't do that, right?  One big
> problem is that we don't have any way to prevent non-temporary
> tablespaces from being created on transient storage.  I wonder if we
> should document this restriction, but it seems awkward to do.

It depends what you mean by allowing the tablespace to start up empty.
It must not be empty once users can run queries, since the catalogs will still
have entries for the tablespace and its dependent objects.  So, what must
happen is that during startup the tablespace and its temporary tables and
indexes get recreated if they are missing.

Mark Dilger

-- 
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] Use of non-restart-safe storage by temp_tablespaces

[Mark Dilger]

> On May 29, 2017, at 11:53 AM, Bruce Momjian <br...@momjian.us> wrote:
> 
> Right now we don't document that temp_tablespaces can use
> non-restart-safe storage, e.g. /tmp, ramdisks.  Would this be safe? 
> Should we document this?

The only safe way to do temporary tablespaces that I have found is to extend
the grammar to allow CREATE TEMPORARY TABLESPACE, and then refuse
to allow the creation of any non-tempoary table (or index on same) in that
tablespace.  Otherwise, it is too easy to be surprised to discover that your
table contents have gone missing.

If the project wanted to extend the grammar and behavior in this direction,
maybe temp_tablespaces would work that way?  I'm not so familiar with what
the temp_tablespaces GUC is for -- only ever implemented what I described
above.

Mark Dilger


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

[HACKERS] syscache entries out of order

[Mark Dilger]

Peter,

Just FYI, as of 665d1fad99e7b11678b0d5fa24d2898424243cd6, syscache.h
entries are not in alphabetical order, which violates the coding standard
specified in the comment for these entries.  In particular, it is the 
PUBLICATION
and SUBSCRIPTION entries that are mis-ordered.

Sorry for my pedantry.  Patch attached.

Mark Dilger



syscache.patch.0
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] Event triggers + table partitioning cause server crash in current master

[Mark Dilger]

> On May 15, 2017, at 9:37 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> 
> wrote:
> 
> On 2017/05/16 1:18, Mark Dilger wrote:
>> 
>>> On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>> 
>>> I can confirm that this fixes the crash that I was seeing.  I have read
>>> through the patch briefly, but will give it a more thorough review in the
>>> next few hours.
> 
> Thanks a lot for the review.
> 
>> My only negative comment is that your patch follows a precedent of using
>> event trigger commands named for AlterTable for operations other than
>> an ALTER TABLE command.  You clearly are not starting the precedent
>> here, as they are already used for IndexStmt and ViewStmt processing, but
>> expanding the precedent by using them in DefineRelation, where they were
>> not used before, seems to move in the wrong direction.  Either the functions
>> should be renamed to something more general to avoid implying that the
>> function is ALTER TABLE specific, or different functions should be defined
>> and used, or ...?  I am uncertain what the right solution would be.
> 
> Hmm.  I think an alternative to doing what I did in my patch is to get rid
> of calling AlterTableInternal() from DefineRelation() altogether.
> Instead, the required ALTER TABLE commands can be added during the
> transform phase, which will create a new AlterTableStmt and add it to the
> list of things to be done after the relation is defined.  That way,
> ProcessUtilitySlow() takes care of doing the event trigger stuff instead
> of having to worry about it ourselves.  Attached updated patch does that.
> 
> PS: actually, we're talking elsewhere [1] of getting rid of adding
> explicit NOT NULL constraints on range partition keys altogether, so even
> if we apply this patch to fix the bug, there is a good chance that the
> code will go away (of course, the bug won't exist too)
> 
> I divided the patch into 2 parts.
> 
> 0001 fixes the bug you reported (not so directly though as my previous
> patch did)
> 
> 0002 fixes the bug I found while working on this, whereby the content of
> CreateStmt will become invalid when creating partitions which could
> cause crash in certain case

Thanks for your continued efforts.  It is passed midnight here, so I think I 
will
save reviewing your new patches until sometime tomorrow morning.

Since we are passed feature freeze, one option is to keep the patch simple
and not do more than you did in your first patch, though until I review the new
patch, I don't know for sure. 

Mark Dilger  

-- 
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] Hash Functions

[Mark Dilger]

> On May 15, 2017, at 7:48 AM, Jeff Davis <pg...@j-davis.com> wrote:
> 
> On Sun, May 14, 2017 at 6:22 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> You'd have to prohibit a heck of a lot more than that in order for
>> this to work 100% reliably.  You'd have to prohibit CHECK constraints,
>> triggers, rules, RLS policies, and UNIQUE indexes, at the least.  You
>> might be able to convince me that some of those things are useless
>> when applied to partitions, but wanting a CHECK constraint that
>> applies to only one partition seems pretty reasonable (e.g. CHECK that
>> records for older years are all in the 'inactive' state, or whatever).
>> I think getting this to work 100% reliably in all cases would require
>> an impractically large hammer.
> 
> The more I think about it the more I think hash partitions are
> "semi-logical". A check constraint on a single partition of a
> range-partitioned table makes sense, but it doesn't make sense on a
> single partition of a hash-partitioned table.

That depends on whether the user gets to specify the hash function, perhaps
indirectly by specifying a user defined opfamily.  I can imagine clever hash
functions that preserve certain properties of the incoming data, and check
constraints in development versions of the database that help verify the hash
is not violating those properties.

That's not to say such hash functions must be allowed in the hash partitioning
implementation; just that it does make sense if you squint and look a bit 
sideways
at it.

Mark Dilger

-- 
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] Event triggers + table partitioning cause server crash in current master

[Mark Dilger]

> On May 15, 2017, at 6:49 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> I can confirm that this fixes the crash that I was seeing.  I have read
> through the patch briefly, but will give it a more thorough review in the
> next few hours.

My only negative comment is that your patch follows a precedent of using
event trigger commands named for AlterTable for operations other than
an ALTER TABLE command.  You clearly are not starting the precedent
here, as they are already used for IndexStmt and ViewStmt processing, but
expanding the precedent by using them in DefineRelation, where they were
not used before, seems to move in the wrong direction.  Either the functions
should be renamed to something more general to avoid implying that the
function is ALTER TABLE specific, or different functions should be defined
and used, or ...?  I am uncertain what the right solution would be.

Thoughts?

Mark Dilger


-- 
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] Event triggers + table partitioning cause server crash in current master

[Mark Dilger]

> On May 14, 2017, at 11:02 PM, Amit Langote <langote_amit...@lab.ntt.co.jp> 
> wrote:
> 
> On 2017/05/14 12:03, Mark Dilger wrote:
>> Hackers,
>> 
>> I discovered a reproducible crash using event triggers in the current
>> development version, 29c7d5e483acaa74a0d06dd6c70b320bb315.
>> I was getting a crash before this version, and cloned a fresh copy of
>> the sources to be sure I was up to date, so I don't think the bug can be
>> attributed to Andres' commit.  (The prior version I was testing against
>> was heavily modified by me, so I recreated the bug using the latest
>> standard, unmodified sources.)
>> 
>> I create both before and after event triggers early in the regression test
>> schedule, which then fire here and there during the following tests, leading
>> fairly reproducibly to the server crashing somewhere during the test suite.
>> These crashes do not happen for me without the event triggers being added
>> to the tests.  Many tests show as 'FAILED' simply because the logging
>> that happens in the event triggers creates unexpected output for the test.
>> Those "failures" are expected.  The server crashes are not.
>> 
>> The server logs suggest the crashes might be related to partitioned tables.
>> 
>> Please find attached the patch that includes my changes to the sources
>> for recreating this bug.  The logs and regression.diffs are a bit large; let
>> me know if you need them.
>> 
>> I built using the command
>> 
>> ./configure --enable-cassert --enable-tap-tests && make -j4 && make check
> 
> Thanks for the report and providing steps to reproduce.
> 
> It seems that it is indeed a bug related to creating range-partitioned
> tables.  DefineRelation() calls AlterTableInternal() to add NOT NULL
> constraints on the range partition key columns, but the code fails to
> first initialize the event trigger context information.  Attached patch
> should fix that.
> 
> Thanks to the above test case, I also discovered that in the case of
> creating a partition, manipulations performed by MergeAttributes() on the
> input schema list may cause it to become invalid, that is, the List
> metadata (length) will no longer match the reality, because while the
> ListCells are deleted from the input list, the List pointer passed to
> list_delete_cell does not point to the same list.  This caused a crash
> when the CreateStmt in question was subsequently passed to copyObject,
> which tried to access CreateStmt.tableElts that has become invalid as just
> described.  The attached patch also takes care of that.

I can confirm that this fixes the crash that I was seeing.  I have read
through the patch briefly, but will give it a more thorough review in the
next few hours.

Many thanks for your attention on this!

Mark Dilger

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

[HACKERS] Event triggers + table partitioning cause server crash in current master

[Mark Dilger]

Hackers,

I discovered a reproducible crash using event triggers in the current
development version, 29c7d5e483acaa74a0d06dd6c70b320bb315.
I was getting a crash before this version, and cloned a fresh copy of
the sources to be sure I was up to date, so I don't think the bug can be
attributed to Andres' commit.  (The prior version I was testing against
was heavily modified by me, so I recreated the bug using the latest
standard, unmodified sources.)

I create both before and after event triggers early in the regression test
schedule, which then fire here and there during the following tests, leading
fairly reproducibly to the server crashing somewhere during the test suite.
These crashes do not happen for me without the event triggers being added
to the tests.  Many tests show as 'FAILED' simply because the logging
that happens in the event triggers creates unexpected output for the test.
Those "failures" are expected.  The server crashes are not.

The server logs suggest the crashes might be related to partitioned tables.

Please find attached the patch that includes my changes to the sources
for recreating this bug.  The logs and regression.diffs are a bit large; let
me know if you need them.

I built using the command

./configure --enable-cassert --enable-tap-tests && make -j4 && make check

Mark Dilger



to_reproduce_bug.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] idea: custom log_line_prefix components besides application_name

[Mark Dilger]

> On May 9, 2017, at 3:14 PM, Chapman Flack <c...@anastigmatix.net> wrote:
> 
> On 05/09/2017 01:25 PM, Mark Dilger wrote:
> 
>> Consensus, no, but utility, yes.
>> 
>> In three tier architectures there is a general problem that the database
>> role used by the middle tier to connect to the database does not entail
>> information about the user who, such as a visitor to your website, made
>> the request of the middle tier.  Chapman wants this information so he
>> can include it in the logs, but stored procedures that work in support
>> of the middle tier might want it for locale information, etc.  As things
>> currently stand, there is no good way to get this passed all the way down
>> into the database stored procedure that needs it, given that you are
>> typically calling down through third party code that doesn't play along.
> 
> I like this framing.
> 
> Clearly a good part of the story is outside of PostgreSQL proper, and
> has to be written elsewhere. There could be a picture like this:
> 
> middle tier receiving request (webapp?) - knows user/origin info
>   |
>   V
> third-party code (rails? web2py? spring?) - doesn't play along
>   |
>   V
> PQ protocol driver (pg? psycopg2? pgjdbc?) - could offer support
>   .
>   .
>   V
> PostgreSQL (what to do here?)
> 
> 
> What to do on the client side of the . . > can only be suggested and
> would have to be independently implemented by several drivers, but
> I could imagine a driver offering some API to tuck a bit of
> application-specific data into some form of thread-local storage.
> In the picture above, the top layer, where the user/origin info
> is known, would need a small modification to call that driver API
> and provide that info. The request could then be processed on down
> through the third-party layer(s) that don't play along. When
> it reaches the driver, something magic will happen to forward
> the thread-local preserved information on to PostgreSQL along with
> the query.
> 
> That of course isn't enough if the intervening layers that don't
> play along use thread pools, and the request could ultimately
> reach the driver on a different thread. But for the simple case
> it gives an idea.
> 
> As to how the driver then propagates the info to PostgreSQL, seems
> to me it could generate a SET in front of the actual query. Most or
> all of what would be needed in PostgreSQL might be possible in an
> extension, which I could try my hand at writing. Here's the idea:
> 
> The extension would define one or more custom GUCs, with flags /
> check hooks to enforce strict limits on when and how they can be set.
> 
> If the client stack is using a simple connection-per-request
> approach, they could just be PGC_BACKEND, and the client part of
> the picture could just be that the top layer supplies them as
> options= in the conninfo string, which various drivers already
> support.
> 
> But if connections may be pooled and re-used for different identities
> and origins, that isn't enough. So the extension would provide
> a function that can be called once in the session, returning
> a random magic cookie. The driver itself would call this function
> upon connecting, and save the cookie in a per-connection
> private variable. Code above the driver in the stack would have
> no access to it, as the function can't be called a second time,
> and so could not spoof identities just by sending arbitrary SET
> commands. The extension would reject any attempts to set or reset
> those GUCs unless accompanied by the cookie.
> 
> Stored procedures could then look at those GUCs for locale / identity
> / origin information and trust that they haven't been spoofed by
> injected commands.
> 
> If there were such a thing as a log_line_prefix_hook, then such an
> extension could also support my original idea and add some new
> escapes to log the added information. But there doesn't seem to be
> such a hook at present. Or, if there were simply a %{name-of-GUC}
> escape supported in log_line_prefix, nothing more would even be
> needed.
> 
> Does this sound workable?

I don't have any positive expectation that the postgres community will go
along with any of this, but just from my point of view, the cleaner way to
do what you are proposing is something like setting a session variable.

In your middle tier java application or whatever, you'd run something like

SET SESSION ON BEHALF OF 'joe user'

And then when you reuse that connection for the next web page you
would do something like that again:

SET SESSION ON BEHALF OF "anonymous from 1.2.3.4"

and so forth.  You wouldn't have to worry about threads since this is
being handled

Re: [HACKERS] idea: custom log_line_prefix components besides application_name

[Mark Dilger]

> On May 9, 2017, at 9:48 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> David Fetter <da...@fetter.org> writes:
>> On Fri, May 05, 2017 at 02:20:26PM -0400, Robert Haas wrote:
>>> On Thu, May 4, 2017 at 10:59 AM, Chapman Flack <c...@anastigmatix.net> 
>>> wrote:
>>>> invalid input syntax for integer: "21' && 1=2)) Uni/**/ON
>>>> SEl/**/eCT 0x646665743166657274,0x646665743266657274,
>>>> 0x646665743366657274 -- "
> 
>>> Now that is choice.  I wonder what specific database system that's
>>> targeting...
> 
>> It could well be targeting some class of pipeline to the database,
>> too, for example one that removes comments and/or un-escapes.
> 
> Yeah.  It's a bit hard to see a database's parser treating "Uni/**/ON"
> as UNION, but if some stack someplace had a keyword check ahead of
> a comment-stripping step, maybe that could do something useful.
> 
>> It occurs to me that psql's habit of stripping out everything on a
>> line that follows a double dash  might be vulnerable in this way, but
>> I wouldn't see such vulnerabilities as super easy to exploit, as psql
>> isn't usually exposed directly to input from the internet.
> 
> I don't think that's a problem: while psql will remove "--" and everything
> following it until newline, it won't remove the newline.  So there's still
> a token boundary there.  If we tried to strip /*...*/ comments we'd have
> to be more careful.
> 
> As far as the actual thread topic goes, I tend to agree with Robert's
> doubt that there's enough utility or consensus for this.

Consensus, no, but utility, yes.

In three tier architectures there is a general problem that the database
role used by the middle tier to connect to the database does not entail
information about the user who, such as a visitor to your website, made
the request of the middle tier.  Chapman wants this information so he
can include it in the logs, but stored procedures that work in support
of the middle tier might want it for locale information, etc.  As things
currently stand, there is no good way to get this passed all the way down
into the database stored procedure that needs it, given that you are
typically calling down through third party code that doesn't play along.

I expect proposals to address this have been made and rejected before.
Is there a word or phrase for this that I can use to search the archives
to read up on the issue?

Thanks,

Mark Dilger

-- 
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] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

[Mark Dilger]

> On May 9, 2017, at 12:18 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> 
> wrote:
> 
> Hi,
> 
> On 2017/05/05 9:38, Mark Dilger wrote:
>> Hackers,
>> 
>> just FYI, I cannot find any regression test coverage of this part of the 
>> grammar, not
>> even in the contrib/ directory or TAP tests.  I was going to submit a patch 
>> to help out,
>> and discovered it is not so easy to do, and perhaps that is why the coverage 
>> is missing.
> 
> I think we could add the coverage by defining a dummy C FDW handler
> function in regress.c.  I see that some other regression tests use C
> functions defined there, such as test_atomic_ops().
> 
> What do you think about the attached patch?  I am assuming you only meant
> to add tests for the code in foreigncmds.c (CREATE/ALTER FOREIGN DATA
> WRAPPER).

Thank you for creating the patch.  I only see one small oversight, which is the
successful case of ALTER FOREIGN DATA WRAPPER ... HANDLER ... is still
not tested.  I added one line for that in the attached modification of your 
patch.

Mark Dilger



0002-Add-some-FDW-HANDLER-DDL-tests.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] no test coverage for ALTER FOREIGN DATA WRAPPER name HANDLER ...

[Mark Dilger]

Hackers,

just FYI, I cannot find any regression test coverage of this part of the 
grammar, not
even in the contrib/ directory or TAP tests.  I was going to submit a patch to 
help out,
and discovered it is not so easy to do, and perhaps that is why the coverage is 
missing.

My apologies for the noise if this is already common knowledge.  Also, if I'm 
wrong,
I'd appreciate a pointer to the test that I am failing to find.

Thanks,

Mark Dilger

-- 
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: Replication lag tracking for walsenders

[Mark Dilger]

> On Apr 23, 2017, at 7:53 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Thomas Munro <thomas.mu...@enterprisedb.com> writes:
>> On Sun, Apr 23, 2017 at 6:01 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Fair enough.  But I'd still like an explanation of why only about
>>> half of the population is showing a failure here.  Seems like every
>>> machine should be seeing the LSN as moving backwards in this test.
>>> So (a) why aren't they all failing, and (b) should we change the
>>> test to make sure every platform sees that happening?
> 
>> Every machine sees the LSN moving backwards, but the code path that
>> had the assertion only reached if it decides to interpolate, which is
>> timing dependent: there needs to be a future sample in the lag
>> tracking buffer, which I guess is not the case in those runs.
> 
> I'm dissatisfied with this explanation because if it's just timing,
> it doesn't seem very likely that some machines would reproduce the
> failure every single time while others never would.  Maybe that can be
> blamed on kernel scheduler vagaries + different numbers of cores, but
> I can't escape the feeling that there's something here we've not
> fully understood.
> 
> While chasing after this earlier today, I turned on some debug logging
> and noted that the standby's reports look like
> 
> 2017-04-23 15:46:46.206 EDT [34829] LOG:  database system is ready to accept 
> read only connections
> 2017-04-23 15:46:46.212 EDT [34834] LOG:  fetching timeline history file for 
> timeline 2 from primary server
> 2017-04-23 15:46:46.212 EDT [34834] LOG:  started streaming WAL from primary 
> at 0/300 on timeline 1
> 2017-04-23 15:46:46.213 EDT [34834] LOG:  sending write 0/302 flush 
> 0/3028470 apply 0/3028470
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  replication terminated by primary 
> server
> 2017-04-23 15:46:46.214 EDT [34834] DETAIL:  End of WAL reached on timeline 1 
> at 0/3028470.
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  sending write 0/3028470 flush 
> 0/3028470 apply 0/3028470
> 2017-04-23 15:46:46.214 EDT [34830] LOG:  new target timeline is 2
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  restarted WAL streaming at 
> 0/300 on timeline 2
> 2017-04-23 15:46:46.228 EDT [34834] LOG:  sending write 0/302 flush 
> 0/3028470 apply 0/3028470
> 
> So you're right that the standby's reported "write" position can go
> backward, but it seems pretty darn odd that the flush and apply
> positions didn't go backward too.  Is there a bug there?
> 
> I remain of the opinion that if we can't tell from the transmitted
> data whether a timeline switch has caused the position to go backward,
> then that's a protocol shortcoming that ought to be fixed.

The recent fix in 546c13e11b29a5408b9d6a6e3cca301380b47f7f has local variable 
overwriteOK
assigned but not used in twophase.c RecoverPreparedTransactions(void).  I'm not 
sure if that's
future-proofing or an oversight.  It seems to be used in other functions.  Just 
FYI.

Mark Dilger



-- 
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_GETARG_GISTENTRY?

[Mark Dilger]

> On Apr 5, 2017, at 1:27 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Apr 5, 2017, at 1:12 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> Mark Dilger <hornschnor...@gmail.com> writes:
>>> I have written a patch to fix these macro definitions across src/ and 
>>> contrib/.
>>> Find the patch, attached.  All regression tests pass on my Mac laptop.
>> 
>> Thanks for doing the legwork on that.  
> 
> You are welcome.
> 
>> This seems a bit late for v10,
>> especially since it's only cosmetic
> 
> Agreed.
> 
>> , but please put it in the first
>> v11 commitfest.
> 
> Done.
> 
>> 
>>> I don't find any inappropriate uses of _P where _PP would be called for.  I 
>>> do,
>>> however, notice that some datatypes' functions are written to use 
>>> PG_GETARG_*_P
>>> where PG_GETARG_*_PP might be more efficient.
>> 
>> Yeah.  I think Noah did some work in that direction already, but I don't
>> believe he claimed to have caught everything.  Feel free to push further.
> 
> Thanks for clarifying.
> 

Here is a small patch for the next open commitfest which handles a case
that Noah's commits 9d7726c2ba06b932f791f2d0cc5acf73cc0b4dca and
3a0d473192b2045cbaf997df8437e7762d34f3ba apparently missed.

Noah, if you left this case out intentionally, sorry for the noise.  I did not
immediately see any reason not to follow your lead for this function.

Mark Dilger



varbit_packed.patch.1
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: Replication lag tracking for walsenders

[Mark Dilger]

> On Apr 22, 2017, at 11:40 AM, Tom Lane  wrote:
> 
> I wrote:
>> Whoa.  This just turned into a much larger can of worms than I expected.
>> How can it be that processes are getting assertion crashes and yet the
>> test framework reports success anyway?  That's impossibly
>> broken/unacceptable.
> 
> I poked into this on my laptop, where I'm able to reproduce both assertion
> failures.  The reason the one in subtrans.c is not being detected by the
> test framework is that it happens late in the run and the test doesn't
> do anything more with that server than shut it down.  "pg_ctl stop"
> actually notices there's a problem; for instance, the log on tern for
> this step shows
> 
> ### Stopping node "master" using mode immediate
> # Running: pg_ctl -D 
> /home/nm/farm/gcc32/HEAD/pgsql.build/src/test/recovery/tmp_check/data_master_xHGA/pgdata
>  -m immediate stop
> pg_ctl: PID file 
> "/home/nm/farm/gcc32/HEAD/pgsql.build/src/test/recovery/tmp_check/data_master_xHGA/pgdata/postmaster.pid"
>  does not exist
> Is server running?
> # No postmaster PID
> 
> However, PostgresNode::stop blithely ignores the failure exit from
> pg_ctl.  I think it should use system_or_bail() not just system_log(),
> so that we'll notice if the server is not running when we expect it
> to be.  It's conceivable that there should also be a "stop_if_running"
> method that allows the case where the server is not running, but so
> far as I can find, no existing TAP test needs such a behavior --- and
> it certainly shouldn't be the default.
> 
> The walsender.c crash is harder to detect because the postmaster very
> nicely recovers and restarts its children.  That's great for robustness,
> but it sucks for testing.  I think that we ought to disable that in
> TAP tests, which we can do by having PostgresNode::init include
> "restart_after_crash = off" in the postgresql.conf modifications it
> applies.  Again, it's conceivable that some tests would not want that,
> but there is no existing test that seems to need crash recovery on,
> and I don't see a good argument for it being default for test purposes.
> 
> As best I've been able to tell, the reason why the walsender.c crash
> is detected when it's detected is that the TAP script chances to try
> to connect while the recovery is happening:
> 
> connection error: 'psql: FATAL:  the database system is in recovery mode'
> while running 'psql -XAtq -d port=57718 host=/tmp/_uP8FKEynq 
> dbname='postgres' -f - -v ON_ERROR_STOP=1' at 
> /home/nm/farm/gcc32/HEAD/pgsql.build/src/test/recovery/../../../src/test/perl/PostgresNode.pm
>  line 1173.
> 
> The window for that is not terribly wide, explaining why the detection
> is unreliable even when the crash does occur.
> 
> In short then, I propose the attached patch to make these cases fail
> more reliably.  We might extend this later to allow the old behaviors
> to be explicitly opted-into, but we don't seem to need that today.
> 
>   regards, tom lane

I pulled fresh sources with your latest commit, 
7d68f2281a4b56834c8e5648fc7da0b73b674c45,
and the tests consistently fail (5 out of 5 attempts) for me on my laptop with:

/Applications/Xcode.app/Contents/Developer/usr/bin/make -C recovery check
for extra in contrib/test_decoding; do 
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C '../../..'/$extra 
DESTDIR='/Users/mark/vanilla/bitoctetlength'/tmp_install install 
>>'/Users/mark/vanilla/bitoctetlength'/tmp_install/log/install.log || exit; done
rm -rf /Users/mark/vanilla/bitoctetlength/src/test/recovery/tmp_check/log
cd . && TESTDIR='/Users/mark/vanilla/bitoctetlength/src/test/recovery' 
PATH="/Users/mark/vanilla/bitoctetlength/tmp_install/usr/local/pgsql/bin:$PATH" 
DYLD_LIBRARY_PATH="/Users/mark/vanilla/bitoctetlength/tmp_install/usr/local/pgsql/lib"
 PGPORT='65432' 
PG_REGRESS='/Users/mark/vanilla/bitoctetlength/src/test/recovery/../../../src/test/regress/pg_regress'
 prove -I ../../../src/test/perl/ -I .  t/*.pl
t/001_stream_rep.pl .. ok 
t/002_archiving.pl ... ok   
t/003_recovery_targets.pl  ok   
t/004_timeline_switch.pl . Bailout called.  Further testing 
stopped:  system pg_ctl failed
FAILED--Further testing stopped: system pg_ctl failed
make[2]: *** [check] Error 255
make[1]: *** [check-recovery-recurse] Error 2
make: *** [check-world-src/test-recurse] Error 2

The first time, I had some local changes, but I've stashed them and am still 
getting the error each of
these next four times.  I'm compiling as follows:

make distclean; make clean; ./configure --enable-cassert --enable-tap-tests 
--enable-depend && make -j4 && make check-world

Are the errors expected now?

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] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 7:41 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 7:35 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> Mark Dilger <hornschnor...@gmail.com> writes:
>>> This is very near where the original crash reported in this thread was 
>>> crashing, probably only
>>> different due to the extra lines of Assert that were added.  Am I missing 
>>> some portion of the
>>> fix that you are testing?  I have only applied the patch that Tom included 
>>> in the previous email.
>> 
>> No, that was the whole patch --- but did you do a full "make clean" and
>> rebuild?  The patch changed NodeTag numbers which would affect the whole
>> tree.
> 
> 

> I'm going to pull completely fresh sources and reapply and retest, though you 
> are welcome
> to review my patch while I do that.

That fixed it.  Thanks.



-- 
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] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 7:35 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>> This is very near where the original crash reported in this thread was 
>> crashing, probably only
>> different due to the extra lines of Assert that were added.  Am I missing 
>> some portion of the
>> fix that you are testing?  I have only applied the patch that Tom included 
>> in the previous email.
> 
> No, that was the whole patch --- but did you do a full "make clean" and
> rebuild?  The patch changed NodeTag numbers which would affect the whole
> tree.

I had trouble applying your patch using

 mark$ patch -p 1 < patch
patching file src/backend/nodes/bitmapset.c
Hunk #1 FAILED at 115.
Hunk #2 FAILED at 146.
Hunk #3 FAILED at 190.
Hunk #4 FAILED at 205.
Hunk #5 FAILED at 229.
Hunk #6 FAILED at 265.
Hunk #7 FAILED at 298.
Hunk #8 FAILED at 324.
Hunk #9 FAILED at 364.
Hunk #10 FAILED at 444.
Hunk #11 FAILED at 463.
Hunk #12 FAILED at 488.
Hunk #13 FAILED at 517.
Hunk #14 FAILED at 554.
Hunk #15 FAILED at 598.
Hunk #16 FAILED at 635.
Hunk #17 FAILED at 665.
Hunk #18 FAILED at 694.
Hunk #19 FAILED at 732.
Hunk #20 FAILED at 770.
Hunk #21 FAILED at 789.
Hunk #22 FAILED at 825.
Hunk #23 FAILED at 853.
Hunk #24 FAILED at 878.
Hunk #25 FAILED at 927.
Hunk #26 FAILED at 981.
Hunk #27 FAILED at 1027.
27 out of 27 hunks FAILED -- saving rejects to file 
src/backend/nodes/bitmapset.c.rej
patching file src/include/nodes/bitmapset.h
Hunk #1 FAILED at 20.
Hunk #2 FAILED at 38.
2 out of 2 hunks FAILED -- saving rejects to file 
src/include/nodes/bitmapset.h.rej
patching file src/include/nodes/nodes.h
Hunk #1 FAILED at 291.
1 out of 1 hunk FAILED -- saving rejects to file src/include/nodes/nodes.h.rej


So I did the patching manually against my copy of the current master,
aba696d1af9a267eee85d69845c3cdeccf788525, and then ran:

make distclean; ./configure --enable-cassert --enable-tap-tests --enable-depend 
&& make -j4 && make check-world

I am attaching my patch, which I admit on closer inspection differs from yours, 
but not
in any way I can tell is relevant:



patch.mark.1
Description: Binary data


I'm going to pull completely fresh sources and reapply and retest, though you 
are welcome
to review my patch while I do that.

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] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 6:48 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> 
>> 
>>> On Apr 8, 2017, at 5:13 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> 
>>> I wrote:
>>>> Robert Haas <robertmh...@gmail.com> writes:
>>>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>>> I think it's pretty dubious to change this, honestly.  Just because it
>>>>> would have caught this one bug doesn't make it an especially valuable
>>>>> thing in general.  Bytes are still not free.
>>> 
>>>> What I think I might do is write a trial patch that turns Bitmapsets
>>>> into Nodes, and see if it catches any other existing bugs.  If it does
>>>> not, that would be good evidence for your position.
>>> 
>>> I made the attached quick-hack patch, and found that check-world
>>> passes just fine with it. 
>> 
>> Not so for me.  I get a failure almost immediately:
> 
> I recant.  Looks like I didn't get the patch applied quite right.  So sorry 
> for the noise.

The regression tests now fail on a number of tests due to a server crash:

2017-04-08 18:55:19.826 PDT [90779] pg_regress/errors STATEMENT:  select 
infinite_recurse();
TRAP: FailedAssertion("!(const Node*)(a))->type) == T_Bitmapset))", File: 
"bitmapset.c", Line: 601)
2017-04-08 18:55:22.487 PDT [90242] LOG:  server process (PID 90785) was 
terminated by signal 6: Abort trap
2017-04-08 18:55:22.487 PDT [90242] DETAIL:  Failed process was running: 
explain (costs off)
select * from onek2 where unique2 = 11 and stringu1 = 'AT';


This is very near where the original crash reported in this thread was 
crashing, probably only
different due to the extra lines of Assert that were added.  Am I missing some 
portion of the
fix that you are testing?  I have only applied the patch that Tom included in 
the previous 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] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 5:13 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> 
>> I wrote:
>>> Robert Haas <robertmh...@gmail.com> writes:
>>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>> I think it's pretty dubious to change this, honestly.  Just because it
>>>> would have caught this one bug doesn't make it an especially valuable
>>>> thing in general.  Bytes are still not free.
>> 
>>> What I think I might do is write a trial patch that turns Bitmapsets
>>> into Nodes, and see if it catches any other existing bugs.  If it does
>>> not, that would be good evidence for your position.
>> 
>> I made the attached quick-hack patch, and found that check-world
>> passes just fine with it. 
> 
> Not so for me.  I get a failure almost immediately:

I recant.  Looks like I didn't get the patch applied quite right.  So sorry for 
the noise.

-- 
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] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 5:13 PM, Tom Lane  wrote:
> 
> I wrote:
>> Robert Haas  writes:
>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  wrote:
>>> I think it's pretty dubious to change this, honestly.  Just because it
>>> would have caught this one bug doesn't make it an especially valuable
>>> thing in general.  Bytes are still not free.
> 
>> What I think I might do is write a trial patch that turns Bitmapsets
>> into Nodes, and see if it catches any other existing bugs.  If it does
>> not, that would be good evidence for your position.
> 
> I made the attached quick-hack patch, and found that check-world
> passes just fine with it. 

Not so for me.  I get a failure almost immediately:

Running in no-clean mode.  Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user "mark".
This user must also own the server process.

The database cluster will be initialized with locales
  COLLATE:  en_US.UTF-8
  CTYPE:en_US.UTF-8
  MESSAGES: C
  MONETARY: en_US.UTF-8
  NUMERIC:  en_US.UTF-8
  TIME: en_US.UTF-8
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory 
/Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... TRAP: FailedAssertion("!(const 
Node*)(a))->type) == T_Bitmapset))", File: "bitmapset.c", Line: 731)
child process was terminated by signal 6: Abort trap
initdb: data directory 
"/Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data" not removed at 
user's request



-- 
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] Uninitialized variable introduced in 3217327053638085d24dd4d276e7c1f7ac2c4c6b

[Mark Dilger]

> On Apr 6, 2017, at 8:33 AM, Peter Eisentraut 
> <peter.eisentr...@2ndquadrant.com> wrote:
> 
> On 4/6/17 10:59, Mark Dilger wrote:
>> Can you perhaps initialize the variable 'address' to suppress the warning?  
>> Thanks.
> 
> A potential fix for this has been pushed.

It works for me.  Thanks again.

Mark Dilger

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

[HACKERS] Uninitialized variable introduced in 3217327053638085d24dd4d276e7c1f7ac2c4c6b

[Mark Dilger]

Peter,

Can you perhaps initialize the variable 'address' to suppress the warning?  
Thanks.

Mark Dilger

tablecmds.c:5984:6: warning: variable 'address' is used uninitialized whenever 
'if' condition is false [-Wsometimes-uninitialized]
if (generatedEl)
^~~
tablecmds.c:5999:9: note: uninitialized use occurs here
return address;
   ^~~
tablecmds.c:5984:2: note: remove the 'if' if its condition is always true
if (generatedEl)
^~~~
tablecmds.c:5936:2: note: variable 'address' is declared here
ObjectAddress address;
^



-- 
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_GETARG_GISTENTRY?

[Mark Dilger]

> On Apr 5, 2017, at 1:12 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>> I have written a patch to fix these macro definitions across src/ and 
>> contrib/.
>> Find the patch, attached.  All regression tests pass on my Mac laptop.
> 
> Thanks for doing the legwork on that.  

You are welcome.

> This seems a bit late for v10,
> especially since it's only cosmetic

Agreed.

> , but please put it in the first
> v11 commitfest.

Done.

> 
>> I don't find any inappropriate uses of _P where _PP would be called for.  I 
>> do,
>> however, notice that some datatypes' functions are written to use 
>> PG_GETARG_*_P
>> where PG_GETARG_*_PP might be more efficient.
> 
> Yeah.  I think Noah did some work in that direction already, but I don't
> believe he claimed to have caught everything.  Feel free to push further.

Thanks for clarifying.



-- 
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_GETARG_GISTENTRY?

[Mark Dilger]

> On Apr 5, 2017, at 9:23 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Robert Haas <robertmh...@gmail.com> writes:
>> On Wed, Mar 29, 2017 at 11:54 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> Andres Freund <and...@anarazel.de> writes:
>>>> we have a good number of '(GISTENTRY *) PG_GETARG_POINTER(n)' in our
>>>> code - looks a bit better & shorter to have PG_GETARG_GISTENTRY(n).
> 
>>> Should be PG_GETARG_GISTENTRY_P to match existing conventions,
>>> otherwise +1
> 
>> I have never quite understood why some of those macros have _P or _PP
>> on the end and others don't.
> 
> _P means "pointer to".  _PP was introduced later to mean "pointer to
> packed (ie, possibly short-header) datum".  Macros that mean to fetch
> pointers to pass-by-ref data, but aren't using either of those naming
> conventions, are violating project conventions, not least because you
> don't know what they're supposed to do with short-header varlena input.
> If I had a bit more spare time I'd run around and change any such macros.
> 
> In short, if you are supposed to write
> 
>   FOO  *val = PG_GETARG_FOO(n);
> 
> then the macro designer blew it, because the name implies that it
> returns FOO, not pointer to FOO.  This should be
> 
>   FOO  *val = PG_GETARG_FOO_P(n);
> 
>   regards, tom lane

I have written a patch to fix these macro definitions across src/ and contrib/.
Find the patch, attached.  All regression tests pass on my Mac laptop.

I don't find any inappropriate uses of _P where _PP would be called for.  I do,
however, notice that some datatypes' functions are written to use PG_GETARG_*_P
where PG_GETARG_*_PP might be more efficient.  Varbit's bitoctetlength function
could detoast only the header ala PG_DETOAST_DATUM_SLICE to return the
octet length, rather than detoasting the whole thing.  But that seems a 
different
issue, and patches to change that might have been rejected in the past so far 
as I
know, so I did not attempt any such changes here.

Mark Dilger



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

[Mark Dilger]

> On Mar 28, 2017, at 1:17 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> 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] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 11:52 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> 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] Monitoring roles patch

[Mark Dilger]

> On Mar 28, 2017, at 11:06 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Mar 28, 2017, at 9:47 AM, Dave Page <dp...@pgadmin.org> 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] 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

[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

[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] Hash support for grouping sets

[Mark Dilger]

> On Mar 23, 2017, at 11:22 AM, Andrew Gierth <and...@tao11.riddles.org.uk> 
> wrote:
> 
>>>>>> "Mark" == Mark Dilger <hornschnor...@gmail.com> writes:
> 
> Mark> You define DiscreteKnapsack to take integer weights and double
> Mark> values, and perform the usual Dynamic Programming algorithm to
> Mark> solve.  But the only place you call this, you pass in NULL for
> Mark> the values, indicating that all the values are equal.  I'm
> Mark> confused why in this degenerate case you bother with the DP at
> Mark> all.  Can't you just load the knapsack from lightest to heaviest
> Mark> after sorting, reducing the runtime complexity to O(nlogn)?  It's
> Mark> been a long day.  Sorry if I am missing something obvious.
> 
> That's actually a very good question.
> 
> (Though in passing I should point out that the runtime cost is going to
> be negligible in all practical cases. Even an 8-way cube (256 grouping
> sets) has only 70 rollups, and a 4-way cube has only 6; the limit of
> 4096 grouping sets was only chosen because it was a nice round number
> that was larger than comparable limits in other database products. Other
> stuff in the grouping sets code has worse time bounds; the
> bipartite-match code used to minimize the number of rollups is
> potentially O(n^2.5) in the number of grouping sets.)
> 
> Thinking about it, it's not at all obvious what we should be optimizing
> for here in the absence of accurate sort costs.

Is there a performance test case where this patch should shine
brightest?  I'd like to load a schema with lots of data, and run
a grouping sets query, both before and after applying the patch,
to see what the performance advantage is.

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] Hash support for grouping sets

[Mark Dilger]

> On Mar 22, 2017, at 8:09 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Mar 22, 2017, at 7:55 AM, Andrew Gierth <and...@tao11.riddles.org.uk> 
>> wrote:
>> 
>> [snip]
>> 
>> This thread seems to have gone quiet - is it time for me to just go
>> ahead and commit the thing anyway? Anyone else want to weigh in?
> 
> Sorry for the delay.  I'll try to look at this today.  Others are most welcome
> to weigh in.

You define DiscreteKnapsack to take integer weights and double values,
and perform the usual Dynamic Programming algorithm to solve.  But
the only place you call this, you pass in NULL for the values, indicating
that all the values are equal.  I'm confused why in this degenerate case
you bother with the DP at all.  Can't you just load the knapsack from
lightest to heaviest after sorting, reducing the runtime complexity to O(nlogn)?
It's been a long day.  Sorry if I am missing something obvious.

I'm guessing you intend to extend the code at some later date to have
different values for items.  Is that right?

I reapplied your patch and reran the regression tests on my laptop and they
all pass.

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

[Mark Dilger]

> On Mar 21, 2017, at 2:13 PM, David Steele <da...@pgmasters.net> wrote:
> 
> Hi Mark,
> 
> On 3/9/17 3:34 PM, Peter Eisentraut wrote:
>> On 3/7/17 18:27, Mark Dilger wrote:
>>> You appear to be using a #define macro to wrap a function of the same name
>>> with the code:
>>> 
>>> #define copyObject(obj) ((typeof(obj)) copyObject(obj))
>> 
>> Yeah, that's a bit silly.  Here is an updated version that changes that.
> 
> Do you know when you'll have a chance to take a look at the updated patch?

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.

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.

I'm not certain that this is (a) merely a bad idea, (b) a different idea than 
what
Peter is proposing, and as such should be submitted independently, or
(c) something that aught to be included in Peter's patch prior to commit.
I only applied this idea to one file, and maybe not completely in that file, 
because
I'd like feedback before going any further along these lines.

mark



ideas_on_top_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] use SQL standard error code for nextval

[Mark Dilger]

> On Mar 9, 2017, at 7:59 AM, Peter Eisentraut 
> <peter.eisentr...@2ndquadrant.com> wrote:
> 
> On 2/28/17 22:15, Peter Eisentraut wrote:
>> The SQL standard defines a separate error code for nextval exhausting
>> the sequence space.  I haven't found any discussion of this in the
>> archives, so it seems this was just not considered or not yet in
>> existence when the error codes were introduced.  Here is a patch to
>> correct it.
> 
> committed

Perhaps you should add something to the release notes.  Somebody could be
testing for the old error code.

Mark Dilger

-- 
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] Hash support for grouping sets

[Mark Dilger]

> On Mar 8, 2017, at 9:40 AM, Andrew Gierth <and...@tao11.riddles.org.uk> wrote:
> 
>>>>>> "Mark" == Mark Dilger <hornschnor...@gmail.com> writes:
> 
> Mark> Hi Andrew,
> 
> Mark> Reviewing the patch a bit more, I find it hard to understand the
> Mark> comment about passing -1 as a flag for finalize_aggregates.  Any
> Mark> chance you can spend a bit more time word-smithing that code
> Mark> comment?
> 
> Sure.
> 
> How does this look for wording (I'll incorporate it into an updated
> patch later):

Much better.  Thanks!

> /*
> * Compute the final value of all aggregates for one group.
> *
> * This function handles only one grouping set at a time.  But in the hash
> * case, it's the caller's responsibility to have selected the set already, and
> * we pass in -1 as currentSet here to flag that; this also changes how we
> * handle the indexing into AggStatePerGroup as explained below.
> *
> * Results are stored in the output econtext aggvalues/aggnulls.
> */
> static void
> finalize_aggregates(AggState *aggstate,
>   AggStatePerAgg peraggs,
>   AggStatePerGroup pergroup,
>   int currentSet)
> {
>   ExprContext *econtext = aggstate->ss.ps.ps_ExprContext;
>   Datum  *aggvalues = econtext->ecxt_aggvalues;
>   bool   *aggnulls = econtext->ecxt_aggnulls;
>   int aggno;
>   int transno;
> 
>   /*
>* If currentSet >= 0, then we're doing sorted grouping, and pergroup 
> is an
>* array of size numTrans*numSets which we have to index into using
>* currentSet in addition to transno. The caller may not have selected 
> the
>* set, so we do that.
>*
>* If currentSet < 0, then we're doing hashed grouping, and pergroup is 
> an
>* array of only numTrans items (since for hashed grouping, each 
> grouping
>* set is in a separate hashtable).  We rely on the caller having done
>* select_current_set, and we fudge currentSet to 0 in order to make the
>* same indexing calculations work as for the grouping case.
>*/
>   if (currentSet >= 0)
>   select_current_set(aggstate, currentSet, false);
>   else
>   currentSet = 0;
> 
> 
> -- 
> Andrew (irc:RhodiumToad)



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

Re: [HACKERS] Hash support for grouping sets

[Mark Dilger]

Hi Andrew,

Reviewing the patch a bit more, I find it hard to understand the comment about
passing -1 as a flag for finalize_aggregates.  Any chance you can spend a bit
more time word-smithing that code comment?


@@ -1559,7 +1647,9 @@ prepare_projection_slot(AggState *aggstate, 
TupleTableSlot *slot, int currentSet
 /*
  * Compute the final value of all aggregates for one group.
  *
- * This function handles only one grouping set at a time.
+ * This function handles only one grouping set at a time.  But in the hash
+ * case, it's the caller's responsibility to have selected the set already, and
+ * we pass in -1 here to flag that and to control the indexing into pertrans.
  *
  * Results are stored in the output econtext aggvalues/aggnulls.
  */
@@ -1575,10 +1665,11 @@ finalize_aggregates(AggState *aggstate,
int aggno;
int transno;

-   Assert(currentSet == 0 ||
-  ((Agg *) aggstate->ss.ps.plan)->aggstrategy != AGG_HASHED);
-
-   aggstate->current_set = currentSet;
+   /* ugly hack for hash */
+   if (currentSet >= 0)
+   select_current_set(aggstate, currentSet, false);
+   else
+   currentSet = 0;


> On Mar 8, 2017, at 8:00 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> 
>> On Mar 8, 2017, at 5:47 AM, Andrew Gierth <and...@tao11.riddles.org.uk> 
>> wrote:
>> 
>>>>>>> "Mark" == Mark Dilger <hornschnor...@gmail.com> writes:
>> 
>> Mark> On my MacBook, `make check-world` gives differences in the
>> Mark> contrib modules:
>> 
>> Thanks! Latest cleaned up version of patch is attached.
> 
> This fixes both the warning and the contrib tests on linux and mac.
> 



-- 
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] Hash support for grouping sets

[Mark Dilger]

> On Mar 8, 2017, at 5:47 AM, Andrew Gierth <and...@tao11.riddles.org.uk> wrote:
> 
>>>>>> "Mark" == Mark Dilger <hornschnor...@gmail.com> writes:
> 
> Mark> On my MacBook, `make check-world` gives differences in the
> Mark> contrib modules:
> 
> Thanks! Latest cleaned up version of patch is attached.

This fixes both the warning and the contrib tests on linux and mac.



-- 
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] Hash support for grouping sets

[Mark Dilger]

> On Mar 8, 2017, at 2:30 AM, Andrew Gierth <and...@tao11.riddles.org.uk> wrote:
> 
>>>>>> "Mark" == Mark Dilger <hornschnor...@gmail.com> writes:
> 
> Mark> On linux/gcc the patch generates a warning in nodeAgg.c that is
> Mark> fairly easy to fix.  Using -Werror to make catching the error
> Mark> easier, I get:
> 
> what gcc version is this exactly?
> 

Linux version 2.6.32-573.18.1.el6.x86_64 (mockbu...@c6b8.bsys.dev.centos.org) 
(gcc version 4.4.7 20120313 (Red Hat 4.4.7-16) (GCC) ) #1 SMP Tue Feb 9 
22:46:17 UTC 2016



-- 
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()

[Mark Dilger]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi Peter,

I like the patch so far, and it passes all the regression tests for me on both 
mac and linux. I
am very much in favor of having more compiler type checking, so +1 from me.

You appear to be using a #define macro to wrap a function of the same name
with the code:

#define copyObject(obj) ((typeof(obj)) copyObject(obj))

I don't necessarily have a problem with that, but it struck me as a bit odd, and
grep'ing through the sources, I don't see anywhere else in the project where 
that
is done for a function of the same number of arguments, and only one other
place where it is done at all:

$ find src/ -type f -name "*.h" -or -name "*.c" | xargs cat | perl -e 
'while(<>) { print if (/^#define (\w+)\b.*\b\1\s*\(\b/); }'
#define copyObject(obj) ((typeof(obj)) copyObject(obj))
#define mkdir(a,b)  mkdir(a)

I'm just flagging that for discussion if anybody thinks it is too magical.
-- 
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] Hash support for grouping sets

[Mark Dilger]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

On linux/gcc the patch generates a warning in nodeAgg.c that is fairly easy
to fix.  Using -Werror to make catching the error easier, I get:

make[3]: Entering directory `/home/mark/postgresql.clean/src/backend/executor'
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -O2 -Werror -I../../../src/include -D_GNU_SOURCE   
-c -o nodeAgg.o nodeAgg.c -MMD -MP -MF .deps/nodeAgg.Po
cc1: warnings being treated as errors
nodeAgg.c: In function ‘ExecAgg’:
nodeAgg.c:2053: error: ‘result’ may be used uninitialized in this function
make[3]: *** [nodeAgg.o] Error 1
make[3]: Leaving directory `/home/mark/postgresql.clean/src/backend/executor'
make[2]: *** [executor-recursive] Error 2
make[2]: Leaving directory `/home/mark/postgresql.clean/src/backend'
make[1]: *** [all-backend-recurse] Error 2
make[1]: Leaving directory `/home/mark/postgresql.clean/src'
make: *** [all-src-recurse] Error 2


There are two obvious choices to silence the warning:

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index f059560..8959f5b 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2066,6 +2066,7 @@ ExecAgg(AggState *node)
break;
case AGG_PLAIN:
case AGG_SORTED:
+   default:
result = agg_retrieve_direct(node);
break;
}

which I like better, because I don't expect it would create
an extra instruction in the compiled code, but also:

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index f059560..eab2605 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2050,7 +2050,7 @@ lookup_hash_entries(AggState *aggstate)
 TupleTableSlot *
 ExecAgg(AggState *node)
 {
-   TupleTableSlot *result;
+   TupleTableSlot *result = NULL;
 
if (!node->agg_done)
{

-- 
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] Hash support for grouping sets

[Mark Dilger]

> On Mar 7, 2017, at 1:43 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> On my MacBook, `make check-world` gives differences in the contrib modules:

I get the same (or similar -- didn't check) regression failure on CentOS, so 
this
doesn't appear to be MacBook / hardware specific.

Mark Dilger

-- 
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] Hash support for grouping sets

[Mark Dilger]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

On my MacBook, `make check-world` gives differences in the contrib modules:

cat contrib/postgres_fdw/regression.diffs
*** 
/Users/mark/hydra/postgresql.review/contrib/postgres_fdw/expected/postgres_fdw.out
  2017-03-03 13:33:47.0 -0800
--- 
/Users/mark/hydra/postgresql.review/contrib/postgres_fdw/results/postgres_fdw.out
   2017-03-07 13:27:56.0 -0800
***
*** 3148,3163 
  -- Grouping sets
  explain (verbose, costs off)
  select c2, sum(c1) from ft1 where c2 < 3 group by rollup(c2) order by 1 nulls 
last;
! QUERY PLAN
 
! 
---
!  GroupAggregate
!Output: c2, sum(c1)
!Group Key: ft1.c2
!Group Key: ()
!->  Foreign Scan on public.ft1
!  Output: c2, c1
!  Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE ((c2 < 3)) ORDER 
BY c2 ASC NULLS LAST
! (7 rows)
  
  select c2, sum(c1) from ft1 where c2 < 3 group by rollup(c2) order by 1 nulls 
last;
   c2 |  sum   
--- 3148,3166 
  -- Grouping sets
  explain (verbose, costs off)
  select c2, sum(c1) from ft1 where c2 < 3 group by rollup(c2) order by 1 nulls 
last;
!   QUERY PLAN  
! --
!  Sort
!Output: c2, (sum(c1))
!Sort Key: ft1.c2
!->  MixedAggregate
!  Output: c2, sum(c1)
!  Hash Key: ft1.c2
!  Group Key: ()
!  ->  Foreign Scan on public.ft1
!Output: c2, c1
!Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE ((c2 < 3))
! (10 rows)
  
  select c2, sum(c1) from ft1 where c2 < 3 group by rollup(c2) order by 1 nulls 
last;
   c2 |  sum   
***
*** 3170,3185 
  
  explain (verbose, costs off)
  select c2, sum(c1) from ft1 where c2 < 3 group by cube(c2) order by 1 nulls 
last;
! QUERY PLAN
 
! 
---
!  GroupAggregate
!Output: c2, sum(c1)
!Group Key: ft1.c2
!Group Key: ()
!->  Foreign Scan on public.ft1
!  Output: c2, c1
!  Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE ((c2 < 3)) ORDER 
BY c2 ASC NULLS LAST
! (7 rows)
  
  select c2, sum(c1) from ft1 where c2 < 3 group by cube(c2) order by 1 nulls 
last;
   c2 |  sum   
--- 3173,3191 
  
  explain (verbose, costs off)
  select c2, sum(c1) from ft1 where c2 < 3 group by cube(c2) order by 1 nulls 
last;
!   QUERY PLAN  
! --
!  Sort
!Output: c2, (sum(c1))
!Sort Key: ft1.c2
!->  MixedAggregate
!  Output: c2, sum(c1)
!  Hash Key: ft1.c2
!  Group Key: ()
!  ->  Foreign Scan on public.ft1
!Output: c2, c1
!Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE ((c2 < 3))
! (10 rows)
  
  select c2, sum(c1) from ft1 where c2 < 3 group by cube(c2) order by 1 nulls 
last;
   c2 |  sum   
***
*** 3192,3211 
  
  explain (verbose, costs off)
  select c2, c6, sum(c1) from ft1 where c2 < 3 group by grouping sets(c2, c6) 
order by 1 nulls last, 2 nulls last;
!  QUERY PLAN   
   
! 
-
   Sort
 Output: c2, c6, (sum(c1))
 Sort Key: ft1.c2, ft1.c6
!->  GroupAggregate
   Output: c2, c6, sum(c1)
!  Group Key: ft1.c2
!  Sort Key: ft1.c6
!Group Key: ft1.c6
   ->  Foreign Scan on public.ft1
 Output: c2, c6, c1
!Remote SQL: SELECT "C 1", c2, c6 FROM "S 1"."T 1" WHERE ((c2 < 
3)) ORDER BY c2 ASC NULLS LAST
! (11 rows)
  
  select c2, c6, sum(c1) from ft1 where c2 < 3 group by grouping sets(c2, c6) 
order by 1 nulls last, 2 nulls last;
   c2 | c6 |  sum  
--- 3198,3216 
  
  explain (verbose, costs off)
  select c2, c6, sum(c1) from ft1 where c2 < 3 group by grouping sets(c2, c6) 
order by 1 nulls last, 2 nulls last;
! QUERY PLAN

! 
--
   Sort
 Output: c2, c6, (sum(c1))
 Sort Key: ft1.c2, ft1.c6
!->  HashAggregate
   Output: c2, c6, sum(c1)
!   

Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes

[Mark Dilger]

> On Feb 17, 2017, at 3:37 PM, Peter Eisentraut 
> <peter.eisentr...@2ndquadrant.com> wrote:
> 
> On 2/17/17 16:13, Mark Dilger wrote:
>> +  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])
> 
> If your compiler isn't warning about anything with that, then there is
> something wrong with it.


$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr 
--with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
InstalledDir: 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin



-- 
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_recvlogical.c doesn't build with --disable-integer-datetimes

[Mark Dilger]

> On Feb 17, 2017, at 12:21 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Thomas Munro <thomas.mu...@enterprisedb.com> writes:
>> On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> [ pokes around... ]  Ah, that's called COPT, and it's entirely
>>> undocumented :-(.  Probably ought to fix that.
> 
>> One way to set that up is like this:
> 
>> $ cat src/Makefile.custom
>> COPT=-Wall -Werror $(CC_OPT)

Makefile.custom is deprecated per comment in src/Makefile.global, so
you'd at least have to remove or edit that comment.  It was enough to
scare me off using Makefile.custom as a solution for this when I was
working on getting -Werror into all my builds.

> Well, we don't document Makefile.custom either, and probably now is
> not the time to start.  I'm inclined to just explain that you can
> set COPT in the environment of the "make" step to add flags that
> you couldn't or didn't tell configure about.

There is a test in configure.in for whether the compiler is GCC, followed
by additional flags if so.  On my platform (Mac laptop) I was able to add
additional flags against HEAD without any of them actually triggering a
warning (though my last merge from HEAD was a while ago):

diff --git a/configure.in b/configure.in
index b9831bc..d6e7e24 100644
--- a/configure.in
+++ b/configure.in
@@ -446,6 +446,24 @@ fi
 
 if test "$GCC" = yes -a "$ICC" = no; then
   CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith"
+  PGAC_PROG_CC_CFLAGS_OPT([-Wempty-body])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wignored-qualifiers])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wtype-limits])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wuninitialized])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-negative-value])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-include-dirs])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-overflow])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wswitch-default])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wdangling-else])
+  PGAC_PROG_CC_CFLAGS_OPT([-Waggregate-return])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wstrict-prototypes])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-declarations])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wredundant-decls])
+  PGAC_PROG_CC_CFLAGS_OPT([-Winline])
+  PGAC_PROG_CC_CFLAGS_OPT([-Woverlength-strings])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wold-style-definition])
   # These work in some but not all gcc versions
   PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement])
   PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d39d6ca..96fbc60 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -240,7 +240,7 @@ endif # not PGXS
 CC = @CC@
 GCC = @GCC@
 SUN_STUDIO_CC = @SUN_STUDIO_CC@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -Werror
 CFLAGS_VECTOR = @CFLAGS_VECTOR@
 CFLAGS_SSE42 = @CFLAGS_SSE42@


How about we add (some of) these extra warnings, plus -Werror,
in a section that is only active for platforms/compilers where we
know there aren't spurious warnings?  That would make detecting
unintentionally introduced warnings simpler, without the use of
COPT.  Perhaps where the compiler is GCC or CLANG, and the
platform is x86_64 redhat, something like that?

Mark Dilger


-- 
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_recvlogical.c doesn't build with --disable-integer-datetimes

[Mark Dilger]

> On Feb 17, 2017, at 11:40 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> I wrote:
>> Yeah.  I have longfin which is running Apple's clang, and is on a machine
>> that doesn't have much to do otherwise.  I propose to turn on -Werror in
>> its configuration, and to configure a second critter on the same hardware
>> that runs with -Werror as well as --disable-integer-datetimes.  Somebody
>> else should do similarly with a reasonably modern/stable gcc release.
> 
> So I tried doing that by adding -Werror to longfin's CFLAGS environment,
> and it crashed and burned in configure.  The most obvious problem is
> that we didn't bother to supply a prototype for does_int64_work() in
> the probes for 64-bit ints.  However, even after fixing that, configure
> produces totally bollixed output because -Werror breaks many of its
> built-in tests.  We could fix the int64 tests but we have no good way of
> fixing the built-in tests.
> 
> AFAICS, if you want to build with -Werror, you have to configure without
> that and then inject it afterwards.  I wonder how people who use -Werror
> are handling that.  Is it possible to do at all in a buildfarm critter?

I ran into the same problem when adding -Werror, and fixed it as follows:

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d39d6ca..96fbc60 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -240,7 +240,7 @@ endif # not PGXS
 CC = @CC@
 GCC = @GCC@
 SUN_STUDIO_CC = @SUN_STUDIO_CC@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -Werror
 CFLAGS_VECTOR = @CFLAGS_VECTOR@
 CFLAGS_SSE42 = @CFLAGS_SSE42@
 
That adds -Werror to the compilation of sources but not to the compilation
of configure test programs.  I'd be happy to hear if anybody has a cleaner
way of doing this.

Mark Dilger


-- 
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] Macro customizable hashtable / bitmapscan & aggregation perf

[Mark Dilger]

Andres,

Your patch, below, appears to have been applied to master in commit
5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5.  It makes mention of a
function, tuplehash_start_iterate, in a macro, but the function is not
defined or declared, and its signature and intention is not clear.  Is there
any chance you could add some documentation about how this function
is intended to be used and defined?

See InitTupleHashIterator in src/include/nodes/execnodes.h

Thanks,

Mark Dilger

> On Oct 9, 2016, at 4:38 PM, Andres Freund <and...@anarazel.de> wrote:
> 
> Hi,
> 
> Attached is an updated version of the patchset. The main changes are
> - address most of Tomas' feedback
> - address regression test output changes by adding explicit ORDER BYs,
>  in a separate commit.
> - fix issues around hash tables sized up to PG_UINT32_MAX
> - fix a bug in iteration with "concurrent" deletions
> 
>>> We didn't really for dynahash (as it basically assumed a fillfactor of 1
>>> all the time), not sure if changing it won't also cause issues.
>>> 
>> 
>> That's a case of glass half-full vs. half-empty, I guess. If we assumed load
>> factor 1.0, then I see it as accounting for load factor (while you see it as
>> not accounting of it).
> 
> Well, that load factor is almost never achieved, because we'd have grown
> since...  I added a comment about disregarding fill factor and growth
> policy to estimate_hashagg_tablesize, which is actually the place where
> it'd seemingly make sense to handle it.
> 
> Tomas, did you have time to run a benchmark?
> 
> I think this is starting to look good.
> 
> 
> Regards,
> 
> Andres
> <0001-Add-likely-unlikely-branch-hint-macros.patch><0002-Make-regression-tests-less-dependent-on-hash-table-o.patch><0003-Add-a-macro-customizable-hashtable.patch><0004-Use-more-efficient-hashtable-for-tidbitmap.c-to-spee.patch><0005-Use-more-efficient-hashtable-for-execGrouping.c-to-s.patch>
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers



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

Re: [HACKERS] should xlog_outdesc modify its argument?

[Mark Dilger]

> On Sep 27, 2016, at 11:25 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote:
> 
> On 09/28/2016 02:35 AM, Mark Dilger wrote:
>> The function
>> 
>>  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>> 
>> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
>> and after returning a string describing an XLogRecord, it clears the
>> state data in its XLogReaderState argument.  That mixes the read-only
>> semantics of "give me a string that describes this argument" and the
>> read-write semantics of "clear out the value in this argument".
> 
> I don't see where the "clears the state data" is happening. Can you elaborate?

My apologies.  At the bottom of the function, it calls through the function 
pointer

RmgrTable[rmid].rm_desc(buf, record);

which is set up to call various *_desc functions.  I must have chased through
those function pointers incorrectly, as I can't find the problem now that I am
reviewing all those functions.

Sorry for the noise,

Mark Dilger



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

[HACKERS] sloppy handling of pointers

[Mark Dilger]

rangetypes_spgist.c contains a call to detoast a pointer which was
just returned from a detoast operation.  This seems to do no harm,
but is to my mind a sloppy thinko.  Fix attached.

tsgistidx.c contains a detoast of a pointer where detoast of a datum
was likely intended.  Fix attached.

tsrank.c contains some arguably correct casts which I found slightly
less correct than an alternative that I've attached.  I'm pretty indifferent
on this one, and just as happy if it is not included.

Mark Dilger



tidy.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] typedef FileName not const?

[Mark Dilger]

> I think the better fix here is to simply remove the typedef. It doesn't
> seem to have much of a benefit, and makes using correct types harder as
> demonstrated here. We don't even use it internally in fd.c..
> 

Fine by me.



-- 
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] gratuitous casting away const

[Mark Dilger]

Friends,

here is another patch, this time fixing the casting away of const
in the regex code.

Mark Dilger



regex.patch.1
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] typedef FileName not const?

[Mark Dilger]

Friends,

along the lines of other similar emails from me of late,
I tried to avoid casting away const when using the FileName
typedef.  There are several calls where a (const char *) has to
be cast to (char *) due to FileName being typedef'd as
non-const.  But changing the typedef to const doesn't seem to
conflict with any code in the source tree.

Since this may be seen as an external API change, I kept
these changes in their own patch submission, so that it can
be rejected separately if need be.

Mark Dilger



filename.patch.1
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] casting away const in comparators

[Mark Dilger]

Friends,

comparators usually take arguments like

  (const void *a, const void *b)

and do something read-only to a and b.  In our
sources, we typically cast these to something else,
like (char *), and do something read-only.  This 
generates a lot of warnings if using -Wcast-qual.
To fix that, I have converted the casts to not cast
away const.  Please find my changes, attached.

Mark Dilger



comparators.patch.1
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] should xlog_outdesc modify its argument?

[Mark Dilger]

The function

  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);

in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
and after returning a string describing an XLogRecord, it clears the
state data in its XLogReaderState argument.  That mixes the read-only
semantics of "give me a string that describes this argument" and the
read-write semantics of "clear out the value in this argument".  That
seems ok, except that xlog_outdesc is also called by the function

  static void rm_redo_error_callback(const void *arg);

also in xlog.c, which appears to only want the string description of the
argument, and not the side effect of clearing out the value.  Now,
perhaps under exceptional conditions it won't matter one way or the
other if the xlog record gets modified; perhaps it's going to be discarded
anyway.  I didn't look far enough to find out.  But this is the only function
of all the (void)(void *arg) callback functions in the entire postgresql
source tree that modifies its argument.  I discovered this while trying
to convert all the callback function signatures to (void)(const void *),
and this was the only monkey-wrench in the works.

Is this a bug?  Do I just have to live with the unfortunate lack of
orthogonality in the callback mechanisms?  At the very least, there
should perhaps be some documentation about how all this is intended
to work.

Thanks, please find my work-in-progress attempt and constify-ing
these functions attached.

Mark Dilger



callback.patch.wip.1
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] gratuitous casting away const

[Mark Dilger]

> On Sep 22, 2016, at 9:14 AM, Tom Lane  wrote:
> 
> I'd call this kind of a wash, I guess.  I'd be more excited about it if
> the change allowed removal of an actual cast-away-of-constness somewhere.
> 
> I suppose it's a bit of a chicken and egg situation, in that the lack
> of const markings on leaf subroutines discourages use of "const" in
> callers, and you have to start somewhere if you want to make it better.
> But I don't really want to just plaster "const" onto individual functions
> without some larger vision of where we're going and which code is going
> to benefit.  Otherwise it seems like mostly just code churn.
> 
>   regards, tom lane

I have two purposes in doing this.  First, I find the code more self-documenting
this way.  Second, I can get whole directories to compile cleanly without
warnings using the -Wcast-qual flag, where currently that flag results in 
warnings.  That makes it possible to add cast-qual to more individual source
directories' Makefiles than I can currently do while still using -Werror in 
Makefile.global.

Now, I'm not proposing that everybody else needs to have -Wcast-qual.  I'm
just saying that I'd like to be able to have that in my copy of the project.

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] gratuitous casting away const

[Mark Dilger]

> On Sep 20, 2016, at 1:06 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnor...@gmail.com> writes:
>> Would patches to not cast away const be considered?
> 
> In general, yes, but I'm not especially in favor of something like this:
> 
>> bool
>> PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
>> -   Item newtup, Size newsize)
>> +   const char *newtup, Size 
>> newsize)
>> {
> 
> since that seems to be discarding type information in order to add
> "const"; does not seem like a net benefit from here.

The following seems somewhere in between, with ItemPointer
changing to const ItemPointerData *.  I expect you would not care
for this change, but thought I'd check to see where you draw the line:


diff --git a/src/backend/access/gin/ginbulk.c b/src/backend/access/gin/ginbulk.c
index 71c64e4..903b01f 100644
--- a/src/backend/access/gin/ginbulk.c
+++ b/src/backend/access/gin/ginbulk.c
@@ -244,7 +244,7 @@ ginInsertBAEntries(BuildAccumulator *accum,
 static int
 qsortCompareItemPointers(const void *a, const void *b)
 {
-   int res = ginCompareItemPointers((ItemPointer) a, (ItemPointer) b);
+   int res = ginCompareItemPointers((const ItemPointerData *) a, 
(const ItemPointerData *) b);

/* Assert that there are no equal item pointers being sorted */
Assert(res != 0);
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index bf589ab..2e5a7dff 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -968,7 +968,7 @@ extern ItemPointer ginMergeItemPointers(ItemPointerData *a, 
uint32 na,
  * so we want this to be inlined.
  */
 static inline int
-ginCompareItemPointers(ItemPointer a, ItemPointer b)
+ginCompareItemPointers(const ItemPointerData *a, const ItemPointerData *b)
 {
uint64  ia = (uint64) a->ip_blkid.bi_hi << 32 | (uint64) 
a->ip_blkid.bi_lo << 16 | a->ip_posid;
uint64  ib = (uint64) b->ip_blkid.bi_hi << 32 | (uint64) 
b->ip_blkid.bi_lo << 16 | b->ip_posid;




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

[HACKERS] needlessly casting away const in localtime.c

[Mark Dilger]

Friends, per the recent thread "gratuitous casting away const", the
assignment on line 1247 of localtime.c has const lvalue and rvalue,
yet casts through (char *) rather than (const char *).  Fix attached.

Mark Dilger



localtime.c.patch
Description: Binary data

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

[HACKERS] gratuitous casting away const

[Mark Dilger]

Friends,

There are places in the code that cast away const for no apparent
reason, fixed like such:



diff --git a/src/backend/executor/nodeIndexscan.c 
b/src/backend/executor/nodeIndexscan.c
index 3143bd9..fe2cfc2 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -409,8 +409,8 @@ static int
 reorderqueue_cmp(const pairingheap_node *a, const pairingheap_node *b,
 void *arg)
 {
-   ReorderTuple *rta = (ReorderTuple *) a;
-   ReorderTuple *rtb = (ReorderTuple *) b;
+   const ReorderTuple *rta = (const ReorderTuple *) a;
+   const ReorderTuple *rtb = (const ReorderTuple *) b;
IndexScanState *node = (IndexScanState *) arg;
 
return -cmp_orderbyvals(rta->orderbyvals, rta->orderbynulls,



There are also places which appear to cast away const due to using
typedefs that don't include const, which make for a more messy fix,
like such:



diff --git a/src/backend/storage/page/bufpage.c 
b/src/backend/storage/page/bufpage.c
index 73aa0c0..9e157a3 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -1037,7 +1037,7 @@ PageIndexTupleDeleteNoCompact(Page page, OffsetNumber 
offnum)
  */
 bool
 PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
-   Item newtup, Size newsize)
+   const char *newtup, Size 
newsize)
 {
PageHeader  phdr = (PageHeader) page;
ItemId  tupid;
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index ad4ab5f..cd97a1a 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -431,7 +431,7 @@ extern void PageIndexTupleDelete(Page page, OffsetNumber 
offset);
 extern void PageIndexMultiDelete(Page page, OffsetNumber *itemnos, int nitems);
 extern void PageIndexTupleDeleteNoCompact(Page page, OffsetNumber offset);
 extern bool PageIndexTupleOverwrite(Page page, OffsetNumber offnum,
-   Item newtup, Size newsize);
+   const char *newtup, Size 
newsize);
 extern char *PageSetChecksumCopy(Page page, BlockNumber blkno);
 extern void PageSetChecksumInplace(Page page, BlockNumber blkno);



Are these castings away of const consistent with the project's coding standards?
Would patches to not cast away const be considered?

Thanks,

Mark Dilger

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

[HACKERS] inappropriate use of NameGetDatum macro

[Mark Dilger]

Friends,

there are several places in the code where variables defined as
(char *) or as (const char *) are passed to the NameGetDatum()
macro.  I believe it would be better form to use CStringGetDatum()
in these locations.  I am aware that these two macros are internally
the same.

src/backend/commands/proclang.c, line 466.
src/backend/commands/dbcommands.c, lines 1263, 1489, 1606, 1746.

Am I overlooking some reason why the code is correct as is? If not,
I am attaching a patch that applies cleanly for me against master,
compiles, and passes the regression tests.

Thanks,

Mark Dilger



NameGetDatum.patch.1
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] 10.0

[Mark Dilger]

> On Jun 20, 2016, at 1:00 PM, David G. Johnston <david.g.johns...@gmail.com> 
> wrote:
> 
> On Mon, Jun 20, 2016 at 3:08 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> > Do you have a problem with the human form and machine forms of the version 
> > number being different in this respect?  I don't - for me the decision of a 
> > choice for the human form is not influenced by the fact the machine form 
> > has 6 digits (with leading zeros which the human form elides...).
> 
> I don't have a problem with it if humans always use a two part number.  I 
> don't read
> the number 14 as being three parts, nor as being two parts, so it doesn't 
> matter.
> What got me to respond this morning was Josh's comment:
> 
> "Realistically, though, we're more likely to end up with 10.0.1 than 10.1."
> 
> He didn't say "11 than 10.1", he said "10.0.1 than 10.1", which showed 
> that we
> already have a confusion waiting to happen.
> 
> Now, you can try to avoid the confusion by saying that we'll always use all 
> three
> digits of the number rather than just two, or always use two digits rather 
> than three.
> But how do you enforce that? 
> 
> ​You do realize he was referring to machine generated output here?  

No I don't, nor will anyone who finds that via a google search.  That's my 
point.
You core hackers feel perfectly comfortable with that because you understand
what you are talking about.  Hardly anybody else will.

As you suggest, that's my $0.02, and I'm moving on.

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] 10.0

[Mark Dilger]

> On Jun 20, 2016, at 11:30 AM, David G. Johnston <david.g.johns...@gmail.com> 
> wrote:
> 
> On Mon, Jun 20, 2016 at 2:14 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> > On Jun 20, 2016, at 11:06 AM, Joshua D. Drake <j...@commandprompt.com> 
> > wrote:
> >
> > On 06/20/2016 10:45 AM, Mark Dilger wrote:
> >
> >>> Now maybe you have some other idea in mind, but I don't quite
> >>> understand what it is.
> >>
> >> I do, indeed, and here it is:
> >>
> >> When considering whether to *back port* a change, we typically do so
> >> on the basis that bug fixes are back ported, features are not.  In my
> >> proposed versioning scheme, you could back port any feature that is
> >> compatible with the old version, and bump the middle number to alert
> >> users that you've not just back ported a bug fix, but a feature.  Any new
> >> features that are not compatible don't get back ported.
> >>
> >> If you fix a bug on master during development, you can back port that
> >> as well.  But instead of bumping the middle number, you bump the last
> >> number.
> >>
> >> Somebody running a version that is three major versions back could
> >> still get the advantage of new features, so long as those new features
> >> are not incompatible.  It's frequently not nearly so easy to run pg_upgrade
> >> as it is to run rpm -U.  You get downtime either way, but the elapsed
> >> time of that downtime is orders of magnitude different.
> >>
> >> Someone else running that same version from three major versions ago
> >> can take a more conservative policy, and only upgrade bug fixes, and
> >> forego the back ported features.
> >>
> >> You still have one major version release per year.  At that time, you can
> >> also release back-port versions of prior major versions.
> >
> > OFFLIST:
> >
> > You are fighting a losing if noble battle.
> 
> I think all my emails on this subject have been seriously misunderstood.
> Perhaps the problem is that I don't understand some critical issue.  Can
> you please help me understand this part:
> 
> It seems like people want releases, starting with 10.0, to be named things
> like 10.0, 10.1, 10.2,..., but for the purposes of communicating with programs
> like nagios refer to them as 10.0.0, 10.0.1, 10.0.2
> 
> Is that right?
> 
> That's the part that really annoys me, and I keep trying to argue for not 
> doing
> that, and people keep replying to other parts of my messages rather than
> replying to the core part of what I am saying.
> 
> Why would any sensible person want a release to sometimes be called
> "10.4", but the exact same release sometimes referred to as "10.0.4"?
> This is just going to confuse average software users, as they would never
> expect that 10.4 and 10.0.4 are the same thing.
> 
> Have I misunderstood what is being proposed?
> 
> ​The software is only ever going to report 10.0.4 OR 10.4.  The area of 
> concern is that people are going to get annoyed at saying: "that was fixed in 
> 10.0.4​" and so mailing lists and other forums where people write the numbers 
> instead of a computer are going to be littered with: "that was fixed in 10.4".
> 
> So now human speak and machine speak are disjointed.
>  
> ​The machine form output for that release is going to be 14 regardless of 
> the decision to make the human form 10.4 or 10.0.4
> 
> Do you have a problem with the human form and machine forms of the version 
> number being different in this respect?  I don't - for me the decision of a 
> choice for the human form is not influenced by the fact the machine form has 
> 6 digits (with leading zeros which the human form elides...).

I don't have a problem with it if humans always use a two part number.  I don't 
read
the number 14 as being three parts, nor as being two parts, so it doesn't 
matter.
What got me to respond this morning was Josh's comment:

"Realistically, though, we're more likely to end up with 10.0.1 than 10.1."

He didn't say "11 than 10.1", he said "10.0.1 than 10.1", which showed that 
we
already have a confusion waiting to happen.

Now, you can try to avoid the confusion by saying that we'll always use all 
three
digits of the number rather than just two, or always use two digits rather than 
three.
But how do you enforce that?  If in some sense the middle number exists, but is
always zero, then some people will mention it and some won't, with the result 
that
10.0.x and 10.x will be used by different people at different times to refer

Re: [HACKERS] 10.0

[Mark Dilger]

> On Jun 20, 2016, at 11:06 AM, Joshua D. Drake <j...@commandprompt.com> wrote:
> 
> On 06/20/2016 10:45 AM, Mark Dilger wrote:
> 
>>> Now maybe you have some other idea in mind, but I don't quite
>>> understand what it is.
>> 
>> I do, indeed, and here it is:
>> 
>> When considering whether to *back port* a change, we typically do so
>> on the basis that bug fixes are back ported, features are not.  In my
>> proposed versioning scheme, you could back port any feature that is
>> compatible with the old version, and bump the middle number to alert
>> users that you've not just back ported a bug fix, but a feature.  Any new
>> features that are not compatible don't get back ported.
>> 
>> If you fix a bug on master during development, you can back port that
>> as well.  But instead of bumping the middle number, you bump the last
>> number.
>> 
>> Somebody running a version that is three major versions back could
>> still get the advantage of new features, so long as those new features
>> are not incompatible.  It's frequently not nearly so easy to run pg_upgrade
>> as it is to run rpm -U.  You get downtime either way, but the elapsed
>> time of that downtime is orders of magnitude different.
>> 
>> Someone else running that same version from three major versions ago
>> can take a more conservative policy, and only upgrade bug fixes, and
>> forego the back ported features.
>> 
>> You still have one major version release per year.  At that time, you can
>> also release back-port versions of prior major versions.
> 
> OFFLIST:
> 
> You are fighting a losing if noble battle.

I think all my emails on this subject have been seriously misunderstood.
Perhaps the problem is that I don't understand some critical issue.  Can 
you please help me understand this part:

It seems like people want releases, starting with 10.0, to be named things
like 10.0, 10.1, 10.2,..., but for the purposes of communicating with programs
like nagios refer to them as 10.0.0, 10.0.1, 10.0.2

Is that right?

That's the part that really annoys me, and I keep trying to argue for not doing
that, and people keep replying to other parts of my messages rather than
replying to the core part of what I am saying.

Why would any sensible person want a release to sometimes be called
"10.4", but the exact same release sometimes referred to as "10.0.4"?
This is just going to confuse average software users, as they would never
expect that 10.4 and 10.0.4 are the same thing.

Have I misunderstood what is being proposed?

The only reason I talk about using the middle number for this other purpose
is to forestall people assuming that they can elide the middle number of a
three number system, such that it gets elided sometimes but not other times.
I was quite clear in my email this morning that I'm not arguing for a three
number system.  I'm perfectly ok with a two number system.  I just don't want
a "let's confuse everybody by making each and every release have two
different names" system.

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] 10.0

[Mark Dilger]

> On Jun 20, 2016, at 9:38 AM, David G. Johnston <david.g.johns...@gmail.com> 
> wrote:
> 
> On Mon, Jun 20, 2016 at 12:28 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
> 
> > On Jun 20, 2016, at 8:53 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
> >
> >
> > This is not a plea for keeping the three part versioning system.  It's just
> > a plea not to have a 2 part versioning system masquerading as a three
> > part versioning system, or vice versa.
> 
> To clarify my concern, I never want to have to write code like this:
> 
> CASE WHEN pg_version eq '11.1' OR pg_version eq '11.0.1' THEN foo()
>WHEN pg_version eq '11.2' OR pg_version eq '11.0.2' THEN 
> bar()
> 
> or
> 
> if (0 == strcmp(pg_version_string, "11.1") || 0 == 
> strcmp(pg_version_string, "11.0.1"))
> foo();
> else if (0 == strcmp(pg_version_string, "11.2") || 0 == 
> strcmp(pg_version_string, "11.0.2"))
> bar();
> 
> either in sql, perl, c, java, or anywhere else.  As soon as you have two 
> different
> formats for the version string, you get into this hell.  Yeah, ok, you may 
> have
> a sql level function for this, but I'm thinking about applications somewhat 
> removed
> from a direct connection to the database, where you can't be sure which format
> you'll be handed.
> 
> Now you argue for keeping the middle number on pure compatibility grounds.​..

I am not arguing for that.  I'm arguing against having two different versions 
of the
same thing.  If you go with a two part versioning scheme that is sometimes 
written as a
three part versioning scheme, you make every bit of code that deals with it 
from now on
have to deal with both possible versions in order to be robust.

My preference is to have a three part versioning scheme where all three parts 
have
different purposes.  But since the community seems to have no interest in that, 
and
have largely (if not universally) rejected that idea, I'm falling back to 
merely arguing
that if we're going to have a two part versioning system, we should do that 
everywhere,
and if we can't do that everywhere, then we should do that nowhere.

You appear to be arguing that we should have a versioning scheme that is 
sometimes
two parts, and sometimes three parts, but when three parts, always make the 
middle
number zero.  The part of that which bothers me most is not the "always zero" 
part.   It's
the "sometimes two parts, sometimes three parts" part.

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] 10.0

[Mark Dilger]

> On Jun 20, 2016, at 9:43 AM, Robert Haas <robertmh...@gmail.com> wrote:
> 
> On Mon, Jun 20, 2016 at 12:26 PM, Mark Dilger <hornschnor...@gmail.com> wrote:
>>> In practical effect that is exactly what your proposal does.  You just feel 
>>> better because you defined when B is allowed to change even though it never 
>>> should happen based upon our project policy.  And any rare exception can 
>>> justifiably be called a bug fix because, face it, it would only happen if 
>>> someone reports a bug.
>> 
>> Why are you refusing to acknowledge the difference between features that 
>> require a pg_upgrade and features that don't?
> 
> The amount of additional committer work that would be created by
> making that distinction would be large.  Currently, we're on an annual
> release cycle.  Every commit that's not a bug fix gets committed to
> exactly one branch: master.  Inevitably, there are multiple changes
> per cycle - dozens, probably - that change initial catalog contents
> and would therefore require pg_upgrade.
> 
> Suppose we switched to a semi-annual release cycle where every other
> release required a pg_upgrade, so once per year same as now, and the
> other ones did not.  The only way to do that would be to have two
> active development branches, one of which accepts only changes that
> don't bump catversion or xlog_page_magic and the other of which
> accepts changes of all sorts.  Every patch that qualifies for the
> no-pg-upgrade-required branch would have to be committed twice,
> resolving conflicts as necessary.
> 
> Also, over time, the number of supported branches would approximately
> double.  With a five year support window, it's currently about six.
> If you had another set of semi-major releases in between the main set
> of releases, you'd end up with 11 or 12 active branches, which would
> make back-patching significantly more burdensome than currently.
> 
> Now maybe you have some other idea in mind, but I don't quite
> understand what it is. 

I do, indeed, and here it is:

When considering whether to *back port* a change, we typically do so
on the basis that bug fixes are back ported, features are not.  In my
proposed versioning scheme, you could back port any feature that is
compatible with the old version, and bump the middle number to alert
users that you've not just back ported a bug fix, but a feature.  Any new
features that are not compatible don't get back ported.

If you fix a bug on master during development, you can back port that
as well.  But instead of bumping the middle number, you bump the last
number.

Somebody running a version that is three major versions back could
still get the advantage of new features, so long as those new features
are not incompatible.  It's frequently not nearly so easy to run pg_upgrade
as it is to run rpm -U.  You get downtime either way, but the elapsed
time of that downtime is orders of magnitude different.

Someone else running that same version from three major versions ago
can take a more conservative policy, and only upgrade bug fixes, and
forego the back ported features.

You still have one major version release per year.  At that time, you can
also release back-port versions of prior major versions.



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