Re: [HACKERS] Extending COPY TO

[Stephen Frost]

Andrea,

* Andrea Riciputi (andrea.ricip...@gmail.com) wrote:
> My idea was to extend the COPY TO command to accept an EOL option as it 
> already does with the DELIMITER option. To keep it simple we can limit the 
> EOL choice to CR, LF or CRLF to avoid unusual output, and also keep the 
> current behaviour when no EOL option is given. I was also wondering if an EOL 
> option could be useful also for the text output format or not.

Have you considered using COPY TO's 'PROGRAM' option to simply pipe the
output through unix2dos..?

> I spent the weekend reading the COPY command source code and its grammar 
> definition and I think I can patch it by myself, submit the patch here and 
> wait for your review. However before starting this in my spare time I wanted 
> to know if you, as the PG hackers community, would be against a similar 
> proposal for any reason, and if so why.

I'm not particularly against it, though if it can be solved with the
existing 'PROGRAM' capability then it may not make sense to complicate
the COPY code further.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Extending COPY TO

[Heikki Linnakangas]

On 09/23/2014 09:49 AM, Andrea Riciputi wrote:

My idea was to extend the COPY TO command to accept an EOL option as
it already does with the DELIMITER option. To keep it simple we can
limit the EOL choice to CR, LF or CRLF to avoid unusual output, and
also keep the current behaviour when no EOL option is given. I was
also wondering if an EOL option could be useful also for the text
output format or not.


I don't think we want to go down that path. There are plenty of options 
in COPY already, and the more you add, the more complicated it gets. And 
we're never going to be able to satisfy everyone's needs.


I'd suggest doing:

COPY table TO PROGRAM 'unix2dos > /tmp/file'

- Heikki


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

[HACKERS] Extending COPY TO

[Andrea Riciputi]

Hi all,
it’s my first time here, so please let me know if I’m doing something wrong. 
I’m a developer, heavy PG user, but I’ve never hacked it before. Last week at 
work we had to produce a quite big CSV data file which should be used as input 
by another piece of software.

Since the file must be produced on a daily basis, is big, and it contains data 
stored in our PG database, letting PG produce the file itself seemed the right 
approach. Unfortunately the target software is, let say, “legacy” software and 
can only accept CRLF as EOL character. Since our PG is installed on a Linux 
server COPY TO results in a CSV file with LF as EOL, forcing us to pass the 
file a second time to convert EOL, which is inconvenient.

My idea was to extend the COPY TO command to accept an EOL option as it already 
does with the DELIMITER option. To keep it simple we can limit the EOL choice 
to CR, LF or CRLF to avoid unusual output, and also keep the current behaviour 
when no EOL option is given. I was also wondering if an EOL option could be 
useful also for the text output format or not.

I spent the weekend reading the COPY command source code and its grammar 
definition and I think I can patch it by myself, submit the patch here and wait 
for your review. However before starting this in my spare time I wanted to know 
if you, as the PG hackers community, would be against a similar proposal for 
any reason, and if so why.

Thanks in advance,
Andrea

-- 
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] RLS feature has been committed

[Heikki Linnakangas]

On 09/23/2014 09:15 AM, Peter Geoghegan wrote:

On Mon, Sep 22, 2014 at 8:25 PM, Robert Haas  wrote:

Should RLS be reverted, and revisited in a future CF?


IMHO, that would be entirely appropriate.


That seems pretty straightforward, then. I think that it will have to
be reverted.


OTOH, if the patch is actually OK as it was committed, there's no point 
reverting it only to commit it again later. At the end of the day, the 
important thing is that the patch gets sufficient review. Clearly 
Stephen thinks that it did, while you and Andres do not.


To make sure this doesn't just slip by without sufficient review, I'll 
add this to the next commitfest. It's a bit unusual to have a commitfest 
entry for something that's already been committed, but that's fine.


- Heikki



--
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] Commitfest status

[Heikki Linnakangas]

On 09/20/2014 06:54 AM, Michael Paquier wrote:

CF3 is actually over for a couple of days,


There are different opinions on when a commitfest is "over". In my 
opinion, the point of a commitfest is that every patch that someone 
submits gets enough review so that the patch author knows what he needs 
to do next. It's not determined by a date, but by progress.



wouldn't it be better to
bounce back patches marked as "waiting on author" and work on the rest
needing review?


Yep, it's time to do that.

I have now marked those patches that have been in "Waiting on Author" 
state, but have already been reviewed to some extent, as "Returned with 
Feedback".


I kept a two patches:

* Flush buffers belonging to unlogged tables, and
* Function returning the timestamp of last transaction

The first one is a bug-fix, and the second one is stalled by a bug-fix 
that hasn't been applied yet. We should deal with them ASAP.



There are still plenty of patches in "Needs review" state. We got below 
20 at one point, but are back to 24 now. Reviewers: Please *review a 
patch*! We need to get closure to every patch.


Patch authors: Nag the reviewer of your patch. If that doesn't help, 
contact other people who you think would be qualified to review your 
patch, and ask them nicely to review your patch.


- Heikki



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

[HACKERS] Review of GetUserId() Usage

[Stephen Frost]

Greetings,

  While looking through the GetUserId() usage in the backend, I couldn't
  help but notice a few rather questionable cases that, in my view,
  should be cleaned up.

  As a reminder, GetUserId() returns the OID of the user we are
  generally operating as (eg: whatever the 'role' is, as GetUserId()
  respects SET ROLE).  It does NOT include roles which we currently have
  the privileges of (that would be has_privs_of_role()), nor roles which
  we could SET ROLE to (that's is_member_of_role, or check_is_... if you
  want to just error out in failure cases).

  On to the list-

  pgstat_get_backend_current_activity()
Used to decide if the current activity string should be returned or
not.  In my view, this is a clear case which should be addressed
through has_privs_of_role() instead of requiring the user to
SET ROLE to each role they are an inheirited member of to query for
what the other sessions are doing.

  pg_signal_backend()
Used to decide if pg_terminate_backend and pg_cancel_backend are
allowed.  Another case which should be changed over to
has_privs_of_role(), in my view.  Requiring the user to SET ROLE for
roles which they are an inheirited member of is confusing as it's
generally not required.

  pg_stat_get_activity()
Used to decide if the state information should be shared.
My opinion is the same as above- use has_privs_of_role().
There are a number of other functions in pgstatfuncs.c with similar
issues (eg: pg_stat_get_backend_activity(),
pg_stat_get_backend_client_port(), and others).

  Changing these would make things easier for some of our users, I'm
  sure..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] RLS feature has been committed

[Peter Geoghegan]

On Mon, Sep 22, 2014 at 8:25 PM, Robert Haas  wrote:
>> Should RLS be reverted, and revisited in a future CF?
>
> IMHO, that would be entirely appropriate.

That seems pretty straightforward, then. I think that it will have to
be reverted.

> but I do feel that Stephen's feelings of being chastised
> aren't worth the bits they are printed on.

I believe that Stephen intended to convey taking that on the chin, as
he should - up to a point.

> We're here to develop
> software together, not to talk about our feelings; and the quality of
> the software we produce depends on our willingness to follow a set of
> procedures that are or should be well-understood by long-time
> contributors, not on our emotional states.

It also depends on trust, and mutual respect. We'd get very little
done without that.

> It's difficult to imagine a more flagrant violation of process than
> committing a patch without any warning and without even *commenting*
> on the fact that clear objections to commit were made on a public
> mailing list.  If that is allowed to stand, what can we assume other
> than that Stephen, at least, has a blank check to change anything he
> wants, any time he wants, with no veto possible from anyone else?

I think that this is excessive. Do you really believe that Stephen
thinks that, or means to assert that he has a blank check? If you
don't, then why say it? If you do, then the evidence is pretty thin on
the ground. You say "if this is allowed to stand" as if you think that
it might be - I don't think that it will.

If Stephen had a track record of recklessly committing things, then
I'd understand your use of such strong words. I don't think that he
does, though. You're jumping to the worst possible conclusion about
Stephen's motivations.

That having been said, I did go and look at the RLS thread, and
Stephen's commit did seem to be made in quite a bit more haste than I
first imagined. As someone that has an interest in PostgreSQL's
success in general, and as someone that has to wait for a long time to
get serious review of my work, I must say that that does annoy me. I
submitted a patch in March, that was finally marked "Ready for
Committer" by Michael Paquier about 2 months ago.

-- 
Peter Geoghegan


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

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Tomonari Katsumata]

Hi Stephen,

As you said, I'm not good at English, so I'm glad you handle this thread.
I'll wait for the good changing.

Thank you very very much!

regards,
--
Tomonari Katsumata


2014-09-23 14:23 GMT+09:00 Stephen Frost :

> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Stephen Frost  writes:
> > > To clarify- we'll simply swap from (essentially) floor() to ceil() for
> > > handling all GUC_with_unit to internal_unit conversions, document that,
> > > and note it in the release notes as a possible behavior change and move
> > > on.
> >
> > Worksforme.
>
> Great, thanks.  I'll wait a couple days to see if anyone else wants to
> voice a concern.
>
> Tomonari, don't worry about updating the patch (unless you really want
> to) as I suspect I'll be changing around the wording anyway.  No
> offense, please, but I suspect English isn't your first language and
> it'll be simpler for me if I just handle it.  Thanks a lot for the bug
> report and discussion and I'll be sure to credit you for it in the
> commit.
>
> Thanks again!
>
> Stephen
>

Re: [HACKERS] Assertion failure in syncrep.c

[Stephen Frost]

Pavan,

* Pavan Deolasee (pavan.deola...@gmail.com) wrote:
> On Mon, Sep 22, 2014 at 6:50 PM, Stephen Frost  wrote:
> > I agree that it looks like there may be a race condition there- but
> > could you provide the test cases you're working with?  What kind of
> > behavior is it that's making it show up easily for you?
> >
> Nothing special really. Set up a 2-node sync replication on my Mac laptop
> and running pgbench with 10 clients triggers it. As I said, after looking
> at the code and realising that there is a race condition, I tried with with
> gdb to reproduce the race I suspect.

Well, that's a bit concerning.

> Anyways, the attached patch should trigger the race condition for a simple
> query. I'm deliberately making backend to wait to give walsender a chance
> to send outstanding WALs and then making walsender to wait so that
> assertion is triggered in the backend.

Great, thanks, I'll try and look into this tomorrow.

Stephen


signature.asc
Description: Digital signature

[HACKERS] tick buildfarm failure

[Stephen Frost]

All,

  I've been keeping an eye on tick as it failed a day-or-so ago and it
  looks to be related to RLS.  Using a local
  CLFAGS="-DCLOBBER_CACHE_ALWAYS -DRANDOMIZE_ALLOCATED_MEMORY" build, I
  was able to see the regression tests failing in
  check_role_for_policy() due to a pretty clear reset of the memory used
  for the policies.

  Looking through relcache.c, which I have to admit to not being as
  familiar with as some, the issue becomes rather apparent (I believe)-
  RelationClearRelation() hasn't been set up to handle the RLS policies
  specially, as it does for rules and tupledesc.  Fair enough, it's
  reasonably straight-forward to add an equalPolicies() and handle the
  policies the same way- but I'm left wondering if that's actually
  *safe* to do?

  To be a bit more clear- why is it safe to change the contents if the
  equal() function returns 'false'?  I'm guessing the answer is
  "locking"- whereby such a change would imply a lock was acquired on
  the relation by another backend, which shouldn't be possible if we're
  currently working with it, but wanted to double-check that.
  
  I also wonder if we might be better off with a way to identify that
  nothing has actually been changed due to the locks we hold and avoid
  rebuilding the relation cache entry entirely..

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Assertion failure in syncrep.c

[Pavan Deolasee]

On Mon, Sep 22, 2014 at 6:50 PM, Stephen Frost  wrote:

> Pavan,
>
> * Pavan Deolasee (pavan.deola...@gmail.com) wrote:
> > On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee <
> pavan.deola...@gmail.com>
> > wrote:
> > > While running some tests on REL9_2_STABLE branch, I saw an assertion
> > > failure in syncrep.c. The stack trace looks like this:
> > >
> > Any comments on this? I see it very regularly during my pgbench tests.
>
> I agree that it looks like there may be a race condition there- but
> could you provide the test cases you're working with?  What kind of
> behavior is it that's making it show up easily for you?
>
>
Nothing special really. Set up a 2-node sync replication on my Mac laptop
and running pgbench with 10 clients triggers it. As I said, after looking
at the code and realising that there is a race condition, I tried with with
gdb to reproduce the race I suspect.

Anyways, the attached patch should trigger the race condition for a simple
query. I'm deliberately making backend to wait to give walsender a chance
to send outstanding WALs and then making walsender to wait so that
assertion is triggered in the backend.

Hope this helps.

Thanks,
Pavan
-- 
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee


syncrep_assertion_trigger.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] proposal: rounding up time value less than its unit.

[Tom Lane]

David Johnston  writes:
> My original concern was things that are rounded to zero now will not be in
> 9.5 if the non-error solution is chosen.  The risk profile is extremely
> small but it is not theoretically zero.

This is exactly the position I was characterizing as an excessively
narrow-minded attachment to backwards compatibility.  We are trying to
make the behavior better (as in less confusing), not guarantee that it's
exactly the same.  If we are only allowed to change the behavior by
throwing errors in cases where we previously didn't, then we are
voluntarily donning a straitjacket that will pretty much ensure Postgres
doesn't improve any further.

It's important here that this behavior change is being proposed for a
new major release, not for back-patching.  We have never supposed that
postgresql.conf files had to work without any change in new
major releases.

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] proposal: rounding up time value less than its unit.

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > To clarify- we'll simply swap from (essentially) floor() to ceil() for
> > handling all GUC_with_unit to internal_unit conversions, document that,
> > and note it in the release notes as a possible behavior change and move
> > on.
> 
> Worksforme.

Great, thanks.  I'll wait a couple days to see if anyone else wants to
voice a concern.

Tomonari, don't worry about updating the patch (unless you really want
to) as I suspect I'll be changing around the wording anyway.  No
offense, please, but I suspect English isn't your first language and
it'll be simpler for me if I just handle it.  Thanks a lot for the bug
report and discussion and I'll be sure to credit you for it in the
commit.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] proposal: rounding up time value less than its unit.

[David Johnston]

On Tuesday, September 23, 2014, Tom Lane  wrote:

> David G Johnston > writes:
> > Can you either change your mind back to this opinion you held last month
> or
> > commit something you find acceptable - its not like anyone would revert
> > something you commit... :)
>
> Hey, am I not allowed to change my mind :-) ?
>
> Seriously though, the main point I was making before stands: if the
> details of the rounding rule matter, then we messed up on choosing the
> units of the particular GUC.  The question is what are we going to do
> about zero being a special case.
>
> > Everyone agrees non-zero must not round to zero; as long as that happens
> I'm
> > not seeing anyone willing to spending any more effort on the details.
>
> I'm not entirely sure Peter agrees; he wanted to get rid of zero being
> a special case, rather than worry about making the rounding rule safe
> for that case.  But assuming that that's a minority position:
> it seems to me that adding a new error condition is more work for us,
> and more work for users too, and not an especially sane decision when
> viewed from a green-field perspective.  My proposal last month was in
> response to some folk who were arguing for a very narrow-minded
> definition of backwards compatibility ... but I don't think that's
> really where we should go.
>
> regards, tom lane
>

This patch should fix the round-to-zero issue.  If someone wants to get rid
of zero as a special case let them supply a separate patch for that
"improvement".

My original concern was things that are rounded to zero now will not be in
9.5 if the non-error solution is chosen.  The risk profile is extremely
small but it is not theoretically zero.

David J.

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Tom Lane]

Stephen Frost  writes:
> To clarify- we'll simply swap from (essentially) floor() to ceil() for
> handling all GUC_with_unit to internal_unit conversions, document that,
> and note it in the release notes as a possible behavior change and move
> on.

Worksforme.

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] proposal: rounding up time value less than its unit.

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Here's another proposal- how about we support a 'minimum-if-not-zero'
> > option for GUCs more generally, and then throw an error if the user sets
> > the value to a value below that minimum unless they explicitly use zero
> > (to indicate whatever the special meaning of zero for that GUC is)?
> 
> I don't think that the extra complexity in that is worth the effort.

Alright.

> You could maybe talk me into "round to nearest, and then complain if that
> produced zero from nonzero" (in effect, your proposal with the minimum
> always exactly half a unit).  But that seems like mostly extra complexity
> and an extra error case for not much.  Again, *the user shouldn't care*
> what our rounding rule is exactly; if he does, it means the particular
> GUC was misdesigned.

I agree that the user shouldn't have to care, and I agree with your
earlier comment on this thread that having the rounding rules be
different when near zero vs. not-near-zero could be quite confusing.

> We could adopt a straightforward "round to nearest" rule if we changed
> things around enough so that zero was never special, which I think is
> what Peter was really arguing for in the post you cite.  But that strikes
> me as a large amount of work, and a large loss of backwards compatibility,
> in return for (again) not much.

Agreed- I'm a bit concerned about backwards compatibility for all of the
proposals which change the existing rounding rules, but, if the
consensus is that N vs. N+1 shouldn't actually matter for values of
N < X < N+1 (as a one-unit step should be small enough to be not
terribly noticeable), then perhaps we can still move forward with the
ceil() approach as that looks to be the only argument against it.

To clarify- we'll simply swap from (essentially) floor() to ceil() for
handling all GUC_with_unit to internal_unit conversions, document that,
and note it in the release notes as a possible behavior change and move
on.

Thoughts?  Objections?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Tom Lane]

David G Johnston  writes:
> Can you either change your mind back to this opinion you held last month or
> commit something you find acceptable - its not like anyone would revert
> something you commit... :)

Hey, am I not allowed to change my mind :-) ?

Seriously though, the main point I was making before stands: if the
details of the rounding rule matter, then we messed up on choosing the
units of the particular GUC.  The question is what are we going to do
about zero being a special case.

> Everyone agrees non-zero must not round to zero; as long as that happens I'm
> not seeing anyone willing to spending any more effort on the details.

I'm not entirely sure Peter agrees; he wanted to get rid of zero being
a special case, rather than worry about making the rounding rule safe
for that case.  But assuming that that's a minority position:
it seems to me that adding a new error condition is more work for us,
and more work for users too, and not an especially sane decision when
viewed from a green-field perspective.  My proposal last month was in
response to some folk who were arguing for a very narrow-minded
definition of backwards compatibility ... but I don't think that's
really where we should go.

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] proposal: rounding up time value less than its unit.

[David G Johnston]

Tom Lane-2 wrote
> The case where this argument falls down is for "special" values, such as
> where zero means something quite different from the smallest nonzero
> value.  Peter suggested upthread that we should redefine any GUC values
> for which that is true, but (a) I think that loses on backwards
> compatibility grounds, and (b) ISTM zero is probably always special to
> some extent.  A zero time delay for example is not likely to work.
> 
> Maybe we should leave the rounding behavior alone (there's not much
> evidence that rounding in one direction is worse than another; although
> I'd also be okay with changing to round-to-nearest), and confine ourselves
> to throwing an error for the single case that an apparently nonzero input
> value is truncated/rounded to zero as a result of units conversion.

Tom,

Can you either change your mind back to this opinion you held last month or
commit something you find acceptable - its not like anyone would revert
something you commit... :)

Everyone agrees non-zero must not round to zero; as long as that happens I'm
not seeing anyone willing to spending any more effort on the details.

David J.

 



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/proposal-rounding-up-time-value-less-than-its-unit-tp5811102p5820024.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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: rounding up time value less than its unit.

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> This argument doesn't say anything much about which way to round for
>> values that are fractional but larger than the unit size.  I'd probably
>> prefer a "round away from zero" behavior since that seems to be the most
>> consistent rule if we want to avoid silently creating zero values; but
>> I could be talked into something else.

> PeterE argued that rounding away from zero didn't make sense either
> (53f6501b.3090...@gmx.net).  I feel like we're getting trapped by
> examples.

I don't find anything compelling in Peter's argument.  I do agree that
if the GUC unit is hours, and the user tries to set it to 1 second or 1
microsecond, then he probably didn't understand the GUC ... but by that
argument, if the unit is hours and he tries to set it to 1.001 hours, we
should still throw an error.  The point of the GUC units system is to
not be too picky about what the variable's units are, and that that should
be possible as long as the unit is small enough that the user shouldn't
care about the difference between N and N+1 units.  Therefore, I am
entirely unimpressed by examples that hinge on the assumption that the
user *does* care about that; any such example can be rejected on the
grounds that it was our error to use too large a unit for that GUC.
However, as long as we have any cases with special behavior for zero,
we should expect that the user cares about the difference between 0 units
and 1 unit.

> Here's another proposal- how about we support a 'minimum-if-not-zero'
> option for GUCs more generally, and then throw an error if the user sets
> the value to a value below that minimum unless they explicitly use zero
> (to indicate whatever the special meaning of zero for that GUC is)?

I don't think that the extra complexity in that is worth the effort.

You could maybe talk me into "round to nearest, and then complain if that
produced zero from nonzero" (in effect, your proposal with the minimum
always exactly half a unit).  But that seems like mostly extra complexity
and an extra error case for not much.  Again, *the user shouldn't care*
what our rounding rule is exactly; if he does, it means the particular
GUC was misdesigned.

We could adopt a straightforward "round to nearest" rule if we changed
things around enough so that zero was never special, which I think is
what Peter was really arguing for in the post you cite.  But that strikes
me as a large amount of work, and a large loss of backwards compatibility,
in return for (again) not much.

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] RLS feature has been committed

[David G Johnston]

Robert Haas wrote
> It's difficult to imagine a more flagrant violation of process than
> committing a patch without any warning and without even *commenting*
> on the fact that clear objections to commit were made on a public
> mailing list.  If that is allowed to stand, what can we assume other
> than that Stephen, at least, has a blank check to change anything he
> wants, any time he wants, with no veto possible from anyone else?

I'm of a mind to agree that this shouldn't have been committed...but I'm not
seeing where Stephen has done sufficient wrong to justify crucifixion. 
Especially since everything is being done publicly and you are one of the
many people in the position to flex a veto by reverting the patch. 

I see this like a black swan event[1]: needs to be addressed, is thought
provoking, but ultimately shouldn't be treated as something to overreact to
until it happens more frequently.  There are enough checks-and-balances when
it comes to committed code - which in this case is during pre-beta
development - that one should simply allow for a certain level human fallacy
to creep in and need periodic addressing/correcting.

At this point my hindsight says a strictly declaratory statement of "reasons
this is not ready" combined with reverting the patch would have been
sufficient; or even just a "I am going to revert this for these reasons"
post.  The difference between building support for a revert and gathering a
mob is a pretty thin line.  

Subsequent, possibly private, discussion between you and Stephen could then
occur before making any conclusions you form public so that others can learn
from the experience and ponder whether anything could be changed to mitigate
such situations in the future.  

Though I guess if you indeed feel that his actions were truly heinous you
should also then put forth the proposal that his ability to commit be
revoked.  If your not willing to go to that extent then, unless you know
Stephen personally, I'd not assume that public flogging is the best way to
get him to not mess up in the future; but the honest and cordial dialog
about cause/effect is likely to be more productive and less
self-destructing.  Though, to each their own style.

As a committer you have a responsibility to work not only with code but with
those who write the code; and while I myself am not a particularly strong
(or experienced) manager I have enough life experience to give opinions on
leadership.  I won't fault you for being yourself but simply put forth my
own impressions and some ideas to provoke thought.  

I'm also not the one whose efforts were marginalized so don't have the same
emotional attachment to the situation as you do - an attachment that needs
to be recognized because, as I do know from experience, even when you are
right and justified an overreaction makes you come off unfavorably and
doesn't materially improve the situation beyond what it could have been
otherwise.

David J.

1. http://en.wikipedia.org/wiki/Black_swan_theory



--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/RLS-feature-has-been-committed-tp5819983p5820020.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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: rounding up time value less than its unit.

[Stephen Frost]

Hey Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tomonari Katsumata (t.katsumata1...@gmail.com) wrote:
> >> I'm thinking about a method which users get quick awareness it.
> >> Now, it's okay not to change current behavior except non-zero value yields
> >> a zero. A zero rounded down from non-zero gets an error.
> 
> > In general, I'm a fan of this change and will move forward with it,
> > with changes for the above, unless folks object.  Based on the thread so
> > far, this sounds like the right solution and it'd be great to get this
> > simple change done to help move the CF along.
> 
> Hm, I object to a patch that behaves as stated above.  I'm okay with
> silently rounding *up* to the lowest possible nonzero value, eg rounding
> up 7kB to 8kB if the variable's unit is 8kB.  But if we throw an error for
> 7kB and not 8kB, then we are exposing the unit size in a way that the user
> can't ignore.  That seems to me to be antithetical to the entire design
> rationale for GUC units.  More, it doesn't fix the original complaint here,
> which is that the user would be surprised if he specifies 7kB and gets
> some special behavior instead because it's "too close to zero but not
> exactly zero".  7kB should act as though it's not zero.  If the difference
> between 7kB and 8kB is actually user-meaningful, then we chose too coarse
> a unit for the particular GUC, which is not something that a rounding rule
> can paper over.  But the difference between zero and not-zero is
> meaningful regardless of unit choices.

I agree that the difference between 7kB and 8kB shouldn't be meaningful,
but that it can be if it means we end up at zero.  Having different
rounding rules at "near-zero" than in other cases doesn't strike me as
great either though, which is why I thought the error-if-rounded-to-zero
made sense.  As for the user, I'd aruge that they haven't understood the
GUC if they're setting the value down to something which rounds to zero
and an error (yes, even one in the logs that we know few enough folks
read) would be better than where we are currently.

> This argument doesn't say anything much about which way to round for
> values that are fractional but larger than the unit size.  I'd probably
> prefer a "round away from zero" behavior since that seems to be the most
> consistent rule if we want to avoid silently creating zero values; but
> I could be talked into something else.

PeterE argued that rounding away from zero didn't make sense either
(53f6501b.3090...@gmx.net).  I feel like we're getting trapped by
examples.

Here's another proposal- how about we support a 'minimum-if-not-zero'
option for GUCs more generally, and then throw an error if the user sets
the value to a value below that minimum unless they explicitly use zero
(to indicate whatever the special meaning of zero for that GUC is)?
It'd be a larger change, certainly, but I feel like that combined with
the current behavior would address this and possibly other issues
(setting to a value which is low enough to be accepted, but too low to
allow the system to function properly..).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Tom Lane]

Stephen Frost  writes:
> * Tomonari Katsumata (t.katsumata1...@gmail.com) wrote:
>> I'm thinking about a method which users get quick awareness it.
>> Now, it's okay not to change current behavior except non-zero value yields
>> a zero. A zero rounded down from non-zero gets an error.

> In general, I'm a fan of this change and will move forward with it,
> with changes for the above, unless folks object.  Based on the thread so
> far, this sounds like the right solution and it'd be great to get this
> simple change done to help move the CF along.

Hm, I object to a patch that behaves as stated above.  I'm okay with
silently rounding *up* to the lowest possible nonzero value, eg rounding
up 7kB to 8kB if the variable's unit is 8kB.  But if we throw an error for
7kB and not 8kB, then we are exposing the unit size in a way that the user
can't ignore.  That seems to me to be antithetical to the entire design
rationale for GUC units.  More, it doesn't fix the original complaint here,
which is that the user would be surprised if he specifies 7kB and gets
some special behavior instead because it's "too close to zero but not
exactly zero".  7kB should act as though it's not zero.  If the difference
between 7kB and 8kB is actually user-meaningful, then we chose too coarse
a unit for the particular GUC, which is not something that a rounding rule
can paper over.  But the difference between zero and not-zero is
meaningful regardless of unit choices.

This argument doesn't say anything much about which way to round for
values that are fractional but larger than the unit size.  I'd probably
prefer a "round away from zero" behavior since that seems to be the most
consistent rule if we want to avoid silently creating zero values; but
I could be talked into something else.

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] add modulo (%) operator to pgbench

[Stephen Frost]

Fabien,

* Fabien COELHO (coe...@cri.ensmp.fr) wrote:
> >That's not really true.  You can't really add abs(x) or hash(x) right
> >now because the current code only supports this syntax:
> >
> >\set varname operand1 [ operator operand2 ]
> >
> >There's no way to add support for a unary operator with that syntax.
> 
> Hmmm. If you accept a postfix syntax, there is:-)
> 
> But this is not convincing. Adding a unary function with a clean
> syntax indeed requires doing something with the parser.

Based on the discussion so far, it sounds like you're coming around to
agree with Robert (as I'm leaning towards also) that we'd be better off
building a real syntax here instead.  If so, do you anticipate having a
patch to do so in the next few days, or...?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Options OUTPUT_PLUGIN_* controlling format are confusing (Was: Misleading error message in logical decoding)

[Michael Paquier]

On Tue, Sep 23, 2014 at 4:59 AM, Robert Haas  wrote:
> On Thu, Sep 18, 2014 at 11:21 AM, Michael Paquier
>  wrote:
>> On Thu, Sep 18, 2014 at 9:56 AM, Andres Freund  
>> wrote:
>>> On 2014-09-18 09:50:38 -0500, Michael Paquier wrote:
 > Do you see the difference between what your doc patch states and the
 > explanation I've given nearby in this thread?
 Perhaps that's the lack of documentation...
>>>
>>> Man. I've explained it to you about three times. The previous attempts
>>> at doing so didn't seem to help. If my explanations don't explain it so
>>> you can understand it adding them to the docs won't change a thing.
>>> That's why I ask whether you see the difference?
>> Urg sorry for the misunderstanding. The patch stated that this
>> parameter only influences the output of the SQL functions while it
>> defines if "the output plugin requires the output method to support
>> binary data"?
>
> I'm not sure exactly what that sentence means.
>
> But here's the point: every series of bytes is a valid bytea, except
> maybe if it's over 1GB and runs afould of MaxAllocSize.  But a series
> of bytes is only a valid text datum if it's a valid sequence of
> characters according to the database encoding.  We like to think of
> text as being an arbitrary series of bytes, but it isn't.  It can't
> contain any \0 bytes, and it can't contain anything that's invalid in
> the database encoding.  bytea isn't subject to either of those
> restrictions.
>
> So if we were going to have one universal output format for output
> plugins, it would have to be bytea because that, really, can be
> anything.  We could make users convert from that to text or whatever
> they like.  But that's unappealing for several reasons: bytea output
> is printed as unreadable hexademical garbage, and encoding conversions
> are expensive.  So what we do instead is provide a text output method
> and a binary output method.  That way, plugins that want to return
> binary data are free to do so, and output methods that are happy to
> return text can *declare* that what they return is legal text - and
> then we just assume that to be true, and need not do an encoding
> conversion.
Aha, thanks. That's all clear then!
-- 
Michael


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

Re: [HACKERS] RLS feature has been committed

[Robert Haas]

On Mon, Sep 22, 2014 at 9:25 PM, Josh Berkus  wrote:
> The CommitFests were never meant to restrict when a committer could
> commit a patch.  The point of the CFs was to give committers time *off*
> from committing patches.  If a committer wants to commit something
> completely outside of the CF process, they are welcome to, as long as it
> receives adequate review.

Agreed.

> So if there's an argument here, it's whether or not the committed RLS
> patch was adequately reviewed (and if not, if it should be reverted),
> not whether it should have been in the CF or not.

The point here is precisely that nobody other than the authors
reviewed it, and that I specifically asked Stephen to hold off commit
until the next CommitFest because I did not want to drop everything to
review a patch that was posted mid-CommitFest over other patches that
were timely submitted.  Stephen took the technical content that
appeared in that same email, incorporated into the patch, and
committed it shortly thereafter.

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


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

Re: [HACKERS] RLS feature has been committed

[Robert Haas]

On Mon, Sep 22, 2014 at 7:22 PM, Peter Geoghegan  wrote:
> On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund  wrote:
>> This patch has been pushed in a clear violation of established policy.
>>
>> Fundamental pieces of the patch have changed *after* the commitfest
>> started. And there wasn't a recent patch in the commitfest either - the
>> entry was moved over from the last round without a new patch.  It didn't
>> receive independent review (Robert explicitly said his wasn't a full
>> review).  It wasn't marked ready for committer.  The intention to commit
>> wasn't announced publically.  There were *clear* and unaddressed
>> objections to committing the patch as is, by a committer (Robert)
>> nonetheless.
>
> I have no reason to doubt your version of events here

Fortunately, you don't have to take anything on faith.  This is a
public mailing list, and the exact sequence of events is open to
inspection by anyone who cares to take a few minutes to do so.  You
can easily verify whether my statement that I asked Stephen twice to
hold off committing it is correct or not; and you can also verify the
rest of the history that Andres and I recounted.  This is all there in
black and white.

> Should RLS be reverted, and revisited in a future CF?

IMHO, that would be entirely appropriate.  I don't have any idea
whether the patch has remaining bugs, design issues, or security flaws
- and neither does anyone else since the normal review process was
bypassed - but I do feel that Stephen's feelings of being chastised
aren't worth the bits they are printed on.  We're here to develop
software together, not to talk about our feelings; and the quality of
the software we produce depends on our willingness to follow a set of
procedures that are or should be well-understood by long-time
contributors, not on our emotional states.

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list.  If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

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


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

Re: [HACKERS] RLS feature has been committed

[Amit Kapila]

On Tue, Sep 23, 2014 at 6:55 AM, Josh Berkus  wrote:
> On 09/22/2014 04:22 PM, Peter Geoghegan wrote:
> > I have no reason to doubt your version of events here (although
> > Stephen may wish to address what you've said - I'm basing that on his
> > tone elsewhere). I must ask, though: what do you propose to do about
> > it in this instance? He has been chastised. Would you like to make a
> > point of formalizing what are (if I'm not mistaken) currently defacto
> > rules? Should RLS be reverted, and revisited in a future CF?
>
> The CommitFests were never meant to restrict when a committer could
> commit a patch.  The point of the CFs was to give committers time *off*
> from committing patches.  If a committer wants to commit something
> completely outside of the CF process, they are welcome to, as long as it
> receives adequate review.
>
> So if there's an argument here, it's whether or not the committed RLS
> patch was adequately reviewed (and if not, if it should be reverted),

Who decides if the patch is adequately reviewed?

Author, Committer or Reviewer?  In CF, that is comparatively clear
that once Reviewer is satisfied, he marks the patch as
Ready For Committer and then Committer picks up and if he is satisfied
with the review and quality of patch, then he commits the patch or if the
Committer himself is reviewer than in many cases once he is satisfied,
he commits it.

Now in this case, if I understand correctly the story is not
so straightforward.  It seems to me Robert as the Reviewer was not
completely satisfied where as Stephen as the Committer was pretty
much okay with the patch so he went ahead and commits it.

In short, if it is solely at the discretion of Committer that he can
decide if the patch has got adequate Review and he is satisfied
with the quality of patch, then I think what Stephen did in this case
is not wrong (though I am not the one who can decide whether it is
right or wrong, just sharing my thoughts), however if you think Reviewer
also has stake (especially when other Reviewer is also Committer)
in deciding the quality of patch, then Stephen should have waited for
more time (till the Reviewer also gets satisfied).


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] proposal: rounding up time value less than its unit.

[Stephen Frost]

Tomonari,

* Tomonari Katsumata (t.katsumata1...@gmail.com) wrote:
> I'm thinking about a method which users get quick awareness it.
> Now, it's okay not to change current behavior except non-zero value yields
> a zero. A zero rounded down from non-zero gets an error.
> 
> I attached new patch.
> This includes a document about above behavior as Heikki suggested.

Thanks for the updated patch!  I was going through it and there's a few
things-

- Documentation change no longer applies

- Why aren't we worried about a specification of '7kB' being rounded down
  to zero for GUCs which expect at least BLCKSZ?  If the reason is
  "everything that works that way will error on zero anyway today", then
  I don't buy into that argument- there's no sense leaving it
  inconsistent and it would be a potential land-mine to hit later.

- The hint messages look like they need some rewording, at least.

In general, I'm a fan of this change and will move forward with it,
with changes for the above, unless folks object.  Based on the thread so
far, this sounds like the right solution and it'd be great to get this
simple change done to help move the CF along.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Anonymous code block with parameters

[Craig Ringer]

On 09/23/2014 07:20 AM, Petr Jelinek wrote:
>>
>>
>> So, to me, DO vs CREATE FUNCTION has nothing to do with passing
>> arguments and/or returning data.  It has to do with lifespan; single
>> call of the function body only, use DO, otherwise, create a function.
>>
> 
> Actually same thing happened with the DO implementation itself -
> creating anonymous/hidden temporary functions in the background was also
> considered but was decided it's not acceptable (for similar reason temp
> tables were rejected for WITH).
> 
> So we decided at least twice already that this kind of solution is bad,
> I don't know of any change that would invalidate the reasons for
> deciding that way so I don't see why they would suddenly become
> acceptable...

All good points. I was wrong to suggest just going for TEMPORARY
FUNCTION before, there's clearly a useful place for DO with parameters.

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


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

Re: [HACKERS] proposal (9.5) : psql unicode border line styles

[Stephen Frost]

Pavel,

* Pavel Stehule (pavel.steh...@gmail.com) wrote:
> true, sorry, I have a different wording in first design
> 
> fixed

Pushed, thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] RLS feature has been committed

[Josh Berkus]

On 09/22/2014 04:22 PM, Peter Geoghegan wrote:
> I have no reason to doubt your version of events here (although
> Stephen may wish to address what you've said - I'm basing that on his
> tone elsewhere). I must ask, though: what do you propose to do about
> it in this instance? He has been chastised. Would you like to make a
> point of formalizing what are (if I'm not mistaken) currently defacto
> rules? Should RLS be reverted, and revisited in a future CF?

The CommitFests were never meant to restrict when a committer could
commit a patch.  The point of the CFs was to give committers time *off*
from committing patches.  If a committer wants to commit something
completely outside of the CF process, they are welcome to, as long as it
receives adequate review.

So if there's an argument here, it's whether or not the committed RLS
patch was adequately reviewed (and if not, if it should be reverted),
not whether it should have been in the CF or not.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] Should we excise the remnants of borland cc support?

[Tom Lane]

Andres Freund  writes:
> On 2014-09-20 10:03:43 -0400, Andrew Dunstan wrote:
>> I thought the Borland stuff was there only so we could build client
>> libraries for use with things like Delphi.

> FWIW I got offlist reports of two not subscribed people that they simply
> use the normal libpq dll from delphi. Copying it from pgadmin or the pg
> installer.

Whether or not it's really needed to preserve the ability to build libpq
with borland, I'm just about certain that it's never worked to build the
backend with borland (thus explaining the lack of buildfarm members).
So it should be safe enough to strip support appearing in backend-only
header files.

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] WITH CHECK OPTION bug [was RLS Design]

[Stephen Frost]

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> Yeah OK, fair point. Here are some tests that cover that code path.
> I've also thrown in a test with prepared statements, although that
> case was already working, it seemed worth checking.

Applied and backpatched, thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Should we excise the remnants of borland cc support?

[Andres Freund]

On 2014-09-20 10:03:43 -0400, Andrew Dunstan wrote:
> 
> On 09/20/2014 09:24 AM, Andres Freund wrote:
> >Hi,
> >
> >At the moment there's some rememnants of support for borland CC. I don't
> >believe it's likely that any of it still works. I can't remember ever
> >seing a buildfarm animal running it either - not surprising it's ~15
> >years since the last release.
> >Since there's both msvc and mingw support for windows builds - borlands
> >only platform - I see little point in continuing to support it.
> >
> >The reason I'm wondering is that the atomics patch cargo cults forward
> >some stuff specific to borland and I'd rather not do that. And I'd
> >rather be explicit about stopping to do so than slyly doing it.
> >
> 
> I thought the Borland stuff was there only so we could build client
> libraries for use with things like Delphi.
> 
> It might be worth casting the net a little wider to find out if it still has
> any users.

FWIW I got offlist reports of two not subscribed people that they simply
use the normal libpq dll from delphi. Copying it from pgadmin or the pg
installer.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Should we excise the remnants of borland cc support?

[Josh Berkus]

On 09/20/2014 06:24 AM, Andres Freund wrote:
> At the moment there's some rememnants of support for borland CC. I don't
> believe it's likely that any of it still works. I can't remember ever
> seing a buildfarm animal running it either - not surprising it's ~15
> years since the last release.
> Since there's both msvc and mingw support for windows builds - borlands
> only platform - I see little point in continuing to support it.

+1

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] RLS feature has been committed

[Peter Geoghegan]

On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund  wrote:
> This patch has been pushed in a clear violation of established policy.
>
> Fundamental pieces of the patch have changed *after* the commitfest
> started. And there wasn't a recent patch in the commitfest either - the
> entry was moved over from the last round without a new patch.  It didn't
> receive independent review (Robert explicitly said his wasn't a full
> review).  It wasn't marked ready for committer.  The intention to commit
> wasn't announced publically.  There were *clear* and unaddressed
> objections to committing the patch as is, by a committer (Robert)
> nonetheless.

I have no reason to doubt your version of events here (although
Stephen may wish to address what you've said - I'm basing that on his
tone elsewhere). I must ask, though: what do you propose to do about
it in this instance? He has been chastised. Would you like to make a
point of formalizing what are (if I'm not mistaken) currently defacto
rules? Should RLS be reverted, and revisited in a future CF?


-- 
Peter Geoghegan


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

Re: [HACKERS] Anonymous code block with parameters

[Petr Jelinek]

On 22/09/14 22:58, Merlin Moncure wrote:


Meh. Those aren't comparable. TEMPORARY TABLES/INDEXES/... all live
beyond a single statement. What's being discussed here doesn't.


Even if that wasn't true, 'DO' doesn't involve changes to system
catalogs whereas temporary functions would.  With a little imagination
I could come up a with a scenario involving a script of a whole bunch
of repeated trivial DO statements which would involve a lot less
beating on the system catalogs.

When the data-modifying-with feature was considered, an implementation
that relied on temp tables was rejected at least in part because of
system catalog thrash and poorer performance for very trivial queries.

So, to me, DO vs CREATE FUNCTION has nothing to do with passing
arguments and/or returning data.  It has to do with lifespan; single
call of the function body only, use DO, otherwise, create a function.



Actually same thing happened with the DO implementation itself - 
creating anonymous/hidden temporary functions in the background was also 
considered but was decided it's not acceptable (for similar reason temp 
tables were rejected for WITH).


So we decided at least twice already that this kind of solution is bad, 
I don't know of any change that would invalidate the reasons for 
deciding that way so I don't see why they would suddenly become 
acceptable...


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


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

[HACKERS] RLS feature has been committed

[Andres Freund]

Hi,

I'm posting my reply to Stephen's mail at
http://archives.postgresql.org/message-id/20140919163839.GH16422%40tamriel.snowman.net
in a new thread because I think it's a important discussion and many
people probably stopped following the RLS thread at some point.

On 2014-09-19 12:38:39 -0400, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
> > I specifically asked you to hold off on committing this until there
> > was adequate opportunity for review, and explained my reasoning.  You
> > committed it anyway.
>
> Hum- my apologies, I honestly don't recall you specifically asking for
> it to be held off indefinitely.  :(  There was discussion back and
> forth, quite a bit of it with you, and I thank you for your help with
> that and certainly welcome any additional comments.

> > This patch, on the other hand, was massively revised after the start
> > of the CommitFest after many months of inactivity and committed with
> > no thorough review by anyone who was truly independent of the
> > development effort.  It was then committed with no warning over a
> > specific request, from another committer, that more time be allowed
> > for review.
>
> I would not (nor do I feel that I did..) have committed it over a
> specific request to not do so from another committer.  I had been hoping
> that there would be another review coming from somewhere, but there is
> always a trade-off between waiting longer to get a review ahead of a
> commit and having it committed and then available more easily for others
> to work with, review, and generally moving forward.

c.f. 
http://archives.postgresql.org/message-id/CA%2BTgmoaX%2BptioOxx42rxJxsgrvxPfUVyndkpeR0JsRiTeZ36Ng%40mail.gmail.com

> > I'm really disappointed by that.  I feel I'm essentially getting
> > punished for trying to follow what I understand to the process, which
> > has involved me doing huge amounts of review of other people's patches
> > and waiting a very long time to get my own stuff committed, while you
> > bull ahead with your own patches.
> 
> While I wasn't public about it, I actually specifically discussed this
> question with others, a few times even, to try and make sure that I
> wasn't stepping out of line by moving forward.

> That said, I do see that Andres feels similairly.  It certainly wasn't
> my intent to surprise anyone by it but simply to continue to move
> forward- in part, to allow me to properly break from it and work on
> other things, including reviewing other patches in the commitfest.
> I fear I've simply been overly focused on it these past few weeks, for a
> variety of reasons that would likely best be discussed at the pub.

I've now slept a couple days on this. And my position hasn't changed.

This patch has been pushed in a clear violation of established policy.

Fundamental pieces of the patch have changed *after* the commitfest
started. And there wasn't a recent patch in the commitfest either - the
entry was moved over from the last round without a new patch.  It didn't
receive independent review (Robert explicitly said his wasn't a full
review).  It wasn't marked ready for committer.  The intention to commit
wasn't announced publically.  There were *clear* and unaddressed
objections to committing the patch as is, by a committer (Robert)
nonetheless.

It'd be a somewhat different thing if this weren't about a security
sensitive feature, the patch only needed minor cleanup after the start
of the CF, or there at least had been constant public progress over the
last months. Neither is the case.


The circumstances around this being committed make me deeply
uncomfortable. It's setting a very bad precedent. I don't think we want
to go down that route.

> All-in-all, I feel appropriately chastised and certainly don't wish to
> be surprising fellow committers.  Perhaps we can discuss at the dev
> meeting.

I really don't know what you understand under "appropriately chastised"
- but I think there's a pretty obvious way to do handle this
appropriately.

And waiting till the dev meeting obviously makes the whole discussion
moot.

Greetings,

Andres Freund


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

Re: [HACKERS] A mechanism securing web applications in DBMS

[Stephen Frost]

Zhaomo,

* Zhaomo Yang (zhy...@cs.ucsd.edu) wrote:
> You are right. Using unlogged table is a good idea. I'll try it out.
> Thanks for your advice!

Happy to help.  Another option would be to have a custom GUC for this
information.  The issue we have with that currently is that it can be
set by anyone..  Your extension could create one and register functions
which are called when it's set though, and only allow it to be set when
the auth/deauth functions are used.  This would get rid of the need for
any kind of table.

> Currently auth functions are security definer functions. I'm gonna try
> to create a patch using unlogged table + RLS and put it online (e.g.
> this mail list) so that people can try it.

I'd strongly suggest that you look into creating PostgreSQL extensions
and using that mechanism as a way to distribute your security definer
functions and other components of this solution as a single, complete,
package which users can install with just "CREATE EXTENSION ...".  That
might help with both getting others to test and play with your solution.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Anonymous code block with parameters

[Merlin Moncure]

On Mon, Sep 22, 2014 at 2:49 PM, Andres Freund  wrote:
> On 2014-09-22 15:46:48 -0400, Peter Eisentraut wrote:
>> On 9/18/14 7:40 AM, Andres Freund wrote:
>> > I fail to see why that is so much preferrable for you to passing
>> > parameter to DO?
>> >
>> > 1) You need to think about unique names for functions
>> > 2) Doesn't work on HOT STANDBYs
>> > 3) Causes noticeable amount of catalog bloat
>> > 4) Is about a magnitude or two more expensive
>>
>> Doesn't this apply to all temporary objects?  It would also be great to
>> have temporary tables, temporary indexes, temporary triggers, temporary
>> extensions, etc. that don't have the above problems.  I think inventing
>> a separate mechanism for working around each instance of this problem
>> would end up being very confusing.
>
> Meh. Those aren't comparable. TEMPORARY TABLES/INDEXES/... all live
> beyond a single statement. What's being discussed here doesn't.

Even if that wasn't true, 'DO' doesn't involve changes to system
catalogs whereas temporary functions would.  With a little imagination
I could come up a with a scenario involving a script of a whole bunch
of repeated trivial DO statements which would involve a lot less
beating on the system catalogs.

When the data-modifying-with feature was considered, an implementation
that relied on temp tables was rejected at least in part because of
system catalog thrash and poorer performance for very trivial queries.

So, to me, DO vs CREATE FUNCTION has nothing to do with passing
arguments and/or returning data.  It has to do with lifespan; single
call of the function body only, use DO, otherwise, create a function.

merlin


-- 
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] Help to startup

[Alvaro Herrera]

Michael Paquier wrote:
> On Sun, Sep 21, 2014 at 4:52 PM, Craig Ringer  wrote:
> > On 09/17/2014 01:51 AM, Tapan Halani wrote:
> >> Hello everyone..i am new to PostgreSQL project. I had prior experience
> >> with sql+ , with oracle 11g database server. Kindly help me grasp more
> >> about the project.
> >
> > Since you're asking on pgsql-hackers, you're presumably interested in
> > getting involved in developing extensions or feature work?
> >
> > See:
> >
> > http://www.postgresql.org/developer/
> There is a group picture of 2006 :)

Maybe we could turn that into a rotating display of each year's
developers meeting pictures or something like that.

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


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

Re: [HACKERS] jsonb format is pessimal for toast compression

[Josh Berkus]

On 09/19/2014 07:07 AM, Tom Lane wrote:
> Heikki Linnakangas  writes:
>> Tom: You mentioned earlier that your patch fixes some existing bugs. 
>> What were they?
> 
> What I remember at the moment (sans caffeine) is that the routines for
> assembling jsonb values out of field data were lacking some necessary
> tests for overflow of the size/offset fields.  If you like I can apply
> those fixes separately, but I think they were sufficiently integrated with
> other changes in the logic that it wouldn't really help much for patch
> reviewability.

Where are we on this?  Do we have a patch ready for testing?


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] CreateEventTrigStmt copy fix

[Robert Haas]

On Fri, Sep 19, 2014 at 12:09 PM, Petr Jelinek  wrote:
> I was trying to create event trigger inside DO statement inside an extension
> SQL script and noticed that the new event trigger has empty evtevent field.
> After some tinkering with gdb I found out that the memory context switches
> sometimes clear the eventname in CreateEventTrigStmt struct. The reason for
> this is that _copyCreateEventTrigStmt uses COPY_SCALAR_FIELD on eventname
> instead of COPY_STRING_FIELD.
>
> Attached patch fixes this and also the same issue in
> _equalCreateEventTrigStmt.
>
> This should be back-patched to 9.3 where event triggers were introduced.

Done, thanks.

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


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

Re: [HACKERS] Options OUTPUT_PLUGIN_* controlling format are confusing (Was: Misleading error message in logical decoding)

[Robert Haas]

On Thu, Sep 18, 2014 at 11:21 AM, Michael Paquier
 wrote:
> On Thu, Sep 18, 2014 at 9:56 AM, Andres Freund  wrote:
>> On 2014-09-18 09:50:38 -0500, Michael Paquier wrote:
>>> > Do you see the difference between what your doc patch states and the
>>> > explanation I've given nearby in this thread?
>>> Perhaps that's the lack of documentation...
>>
>> Man. I've explained it to you about three times. The previous attempts
>> at doing so didn't seem to help. If my explanations don't explain it so
>> you can understand it adding them to the docs won't change a thing.
>> That's why I ask whether you see the difference?
> Urg sorry for the misunderstanding. The patch stated that this
> parameter only influences the output of the SQL functions while it
> defines if "the output plugin requires the output method to support
> binary data"?

I'm not sure exactly what that sentence means.

But here's the point: every series of bytes is a valid bytea, except
maybe if it's over 1GB and runs afould of MaxAllocSize.  But a series
of bytes is only a valid text datum if it's a valid sequence of
characters according to the database encoding.  We like to think of
text as being an arbitrary series of bytes, but it isn't.  It can't
contain any \0 bytes, and it can't contain anything that's invalid in
the database encoding.  bytea isn't subject to either of those
restrictions.

So if we were going to have one universal output format for output
plugins, it would have to be bytea because that, really, can be
anything.  We could make users convert from that to text or whatever
they like.  But that's unappealing for several reasons: bytea output
is printed as unreadable hexademical garbage, and encoding conversions
are expensive.  So what we do instead is provide a text output method
and a binary output method.  That way, plugins that want to return
binary data are free to do so, and output methods that are happy to
return text can *declare* that what they return is legal text - and
then we just assume that to be true, and need not do an encoding
conversion.

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


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

Re: [HACKERS] Anonymous code block with parameters

[Andres Freund]

On 2014-09-22 15:46:48 -0400, Peter Eisentraut wrote:
> On 9/18/14 7:40 AM, Andres Freund wrote:
> > I fail to see why that is so much preferrable for you to passing
> > parameter to DO?
> > 
> > 1) You need to think about unique names for functions
> > 2) Doesn't work on HOT STANDBYs
> > 3) Causes noticeable amount of catalog bloat
> > 4) Is about a magnitude or two more expensive
> 
> Doesn't this apply to all temporary objects?  It would also be great to
> have temporary tables, temporary indexes, temporary triggers, temporary
> extensions, etc. that don't have the above problems.  I think inventing
> a separate mechanism for working around each instance of this problem
> would end up being very confusing.

Meh. Those aren't comparable. TEMPORARY TABLES/INDEXES/... all live
beyond a single statement. What's being discussed here doesn't.

Greetings,

Andres Freund

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


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

Re: [HACKERS] Anonymous code block with parameters

[Peter Eisentraut]

On 9/18/14 7:40 AM, Andres Freund wrote:
> I fail to see why that is so much preferrable for you to passing
> parameter to DO?
> 
> 1) You need to think about unique names for functions
> 2) Doesn't work on HOT STANDBYs
> 3) Causes noticeable amount of catalog bloat
> 4) Is about a magnitude or two more expensive

Doesn't this apply to all temporary objects?  It would also be great to
have temporary tables, temporary indexes, temporary triggers, temporary
extensions, etc. that don't have the above problems.  I think inventing
a separate mechanism for working around each instance of this problem
would end up being very confusing.



-- 
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] END_OF_RECOVERY shutdowns and ResetUnloggedRelations()

[Robert Haas]

On Thu, Sep 18, 2014 at 4:31 AM, Andres Freund  wrote:
> On 2014-09-12 14:44:48 -0400, Robert Haas wrote:
>> On Fri, Sep 12, 2014 at 7:22 AM, Andres Freund  
>> wrote:
>> > What I like even less is that pg_control is actually marked as
>> > DB_SHUTDOWNED due to END_OF_RECOVERY. That's just plain wrong. Obviously
>> > the database was *NOT* shutdown peacefully. I don't see active bugs due
>> > it besides this, but I think it's likely to either have or create futher
>> > ones.
>>
>> I agree.  The database clearly isn't shut down at end of recovery;
>> it's still active and we're still doing things to it.  If we crash at
>> that point, we need to go back into recovery on restart.  This seems
>> open and shut, but maybe I'm missing something.  Why shouldn't we fix
>> *that*?
>
> Well, I think we might want to do both. There doesn't seem to be a
> reason to *not* do the ResetUnloggedRelation(UNLOGGED_RELATION_INIT)
> around the ShutdownWalRcv(). That seems much closer where it, for me,
> logically belongs. And it'd fix the concrete problem.

That might be all right.  I've booted enough things in this area of
the code to not have a lot of confidence in my ability to not boot
things in this area of the code ... but I don't see a problem with it.
It does seem a little odd to do that before checking whether we
reached the minimum recovery point, or deciding whether to assign a
new TLI, but I don't see a concrete problem with putting it there.

>> With regard to your second email, I agree that
>> ResetUnloggedRelation(UNLOGGED_RELATION_INIT) needs to issue fsyncs.
>
> Good.
>
> The topic reminds me of the fact that I actually think at the very least
> pg_xlog/ and pg_control needs the same treatment. Consider the following
> sequence:
> 1) postgres writes loads of stuff. Lots of it to *not* fsynced WAL
>segments
> 2) postgres crashes and crash recovery happens. Replays *not* fsynced
>WAL
> 3) the OS crashes
> 4) Bad. We now might hava pg_control with a minRecovery that's *later*
>than some potentially unsynced WAL segments
>
> I think the easiest would be to just fsync() the entire data directory
> at the start when ControlFile->state !=
> DB_SHUTDOWNED/DB_SHUTDOWNED_IN_RECOVERY
>
> Note that that's independent of the fsync for unlogged relations.

Seems reasonable.

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


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

Re: [HACKERS] from_collapse_limit considerations

[Tom Lane]

Antonin Houska  writes:
> I noticed that - unlike join_collapse_limit - the from_collapse_limit does not
> enforce maximum length of the top-level list. Shouldn't it do?

How would it do that?  You want it to fail outright if there are more than
N tables?  That seems unhelpful.

> Also, the order of FROM-list items seems to affect the way RTEs are grouped
> into (sub)lists.

Yeah.

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

[HACKERS] from_collapse_limit considerations

[Antonin Houska]

While doing experiments with rather long FROM-lists, I looked closely at the
logic related to from_collapse_limit.

I noticed that - unlike join_collapse_limit - the from_collapse_limit does not
enforce maximum length of the top-level list. Shouldn't it do? Too long
FROM-list can obviously lead to excessive planning time.

Also, the order of FROM-list items seems to affect the way RTEs are grouped
into (sub)lists. In this example, the join of tab_0, tab_1, tab_2, tab_3 gets
expanded into 4 separate RTE refs:

SET from_collapse_limit TO 5;

SELECT  *
FROM
(
(
tab_0
JOIN
tab_1
ON tab_0.id = tab_1.id
)
JOIN
(
tab_2
JOIN
tab_3
ON tab_2.id = tab_3.id
)
ON  tab_1.id = tab_2.id
),
tab_4
JOIN
tab_5
ON tab_4.id = tab_5.id
WHERE   tab_3.id = tab_4.id;

However, in the next example (the JOIN of tab_4 and tab_5 moved to the
beginning of the FROM list), the "bigger join" (tab_0 through tab_3) "comes
too late", so it's inserted as a sub-list.

SET from_collapse_limit TO 5;

SELECT  *
FROM
tab_4
JOIN
tab_5
ON tab_4.id = tab_5.id,
(
(
tab_0
JOIN
tab_1
ON tab_0.id = tab_1.id
)
JOIN
(
tab_2
JOIN
tab_3
ON tab_2.id = tab_3.id
)
ON  tab_1.id = tab_2.id
)
WHERE   tab_3.id = tab_4.id;


Is anything wrong about the idea not to estimate the total length of the FROM
list in deconstruct_recurse and to do additional collapsing later instead? The
patch attached here tries to do so.

I wonder if change of the logic behind from_collapse_limit should be
considered acceptable for users or not: although it improves control over
planning of queries having long FROM-list, it can make some plans of existing
applications worse, unless from_collapse_limit is increased accordingly.

diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
index f88e493..fffc3aa 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -51,6 +51,7 @@ static List *deconstruct_recurse(PlannerInfo *root, Node *jtnode,
 	bool below_outer_join,
 	Relids *qualscope, Relids *inner_join_rels,
 	List **postponed_qual_list);
+static List *collapse_fromlist(List *fromlist);
 static SpecialJoinInfo *make_outerjoininfo(PlannerInfo *root,
    Relids left_rels, Relids right_rels,
    Relids inner_join_rels,
@@ -659,6 +660,9 @@ deconstruct_jointree(PlannerInfo *root)
 	/* Shouldn't be any leftover quals */
 	Assert(postponed_qual_list == NIL);
 
+	/* Create sub-list(s) if the FROM-list appears to be too long.  */
+	result = collapse_fromlist(result);
+
 	return result;
 }
 
@@ -710,7 +714,6 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
 	{
 		FromExpr   *f = (FromExpr *) jtnode;
 		List	   *child_postponed_quals = NIL;
-		int			remaining;
 		ListCell   *l;
 
 		/*
@@ -722,12 +725,10 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
 		*qualscope = NULL;
 		*inner_join_rels = NULL;
 		joinlist = NIL;
-		remaining = list_length(f->fromlist);
 		foreach(l, f->fromlist)
 		{
 			Relids		sub_qualscope;
 			List	   *sub_joinlist;
-			int			sub_members;
 
 			sub_joinlist = deconstruct_recurse(root, lfirst(l),
 			   below_outer_join,
@@ -735,13 +736,14 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
 			   inner_join_rels,
 			   &child_postponed_quals);
 			*qualscope = bms_add_members(*qualscope, sub_qualscope);
-			sub_members = list_length(sub_joinlist);
-			remaining--;
-			if (sub_members <= 1 ||
-list_length(joinlist) + sub_members + remaining <= from_collapse_limit)
-joinlist = list_concat(joinlist, sub_joinlist);
-			else
-joinlist = lappend(joinlist, sub_joinlist);
+
+			/*
+			 * Sub-lists are not created at this stage, as we can't predict how
+			 * many RTEs the possibly following join nodes may contain. Instead,
+			 * length of the top-level list is checked later when
+			 * deconstruct_recurse has finished.
+			 */
+			joinlist = list_concat(joinlist, sub_joinlist);
 		}
 
 		/*
@@ -1013,6 +1015,65 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
 	return joinlist;
 }
 
+
+/*
+ * Ensure that neither the top-level FROM-list nor any its sublist exceeds
+ * from_collapse_limit.
+ */
+static List *
+collapse_fromlist(List *fromlist)
+{
+	List *result = f

Re: [HACKERS] documentation references invalid -A assertion checks option

[Andres Freund]

On 2014-09-22 09:58:49 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2014-09-22 08:43:02 -0500, Merlin Moncure wrote:
> >> Both the documentation
> >> (http://www.postgresql.org/docs/9.4/static/app-postgres.html) and the
> >> --help switch to postgres reference a -A switch to handle assertion
> >> checking.
> 
> > You're probably building master. That option has been removed (by me)
> > since. The 'devel' docs don't show the option anymore unless I missed a
> > place referencing it.
> 
> As Merlin says, HEAD is still doing this:
> 
> $ postgres --help
> postgres is the PostgreSQL server.
> 
> Usage:
>   postgres [OPTION]...
> 
> Options:
>   -A 1|0 enable/disable run-time assert checking
>   ...

Gah, 3bdcf6a5a7555035810e2ba2b8a0fb04dc5c66b8 removed several doc
references about it, but not --help. Will fix.

Greetings,

Andres Freund

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


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

Re: [HACKERS] documentation references invalid -A assertion checks option

[Tom Lane]

Andres Freund  writes:
> On 2014-09-22 08:43:02 -0500, Merlin Moncure wrote:
>> Both the documentation
>> (http://www.postgresql.org/docs/9.4/static/app-postgres.html) and the
>> --help switch to postgres reference a -A switch to handle assertion
>> checking.

> You're probably building master. That option has been removed (by me)
> since. The 'devel' docs don't show the option anymore unless I missed a
> place referencing it.

As Merlin says, HEAD is still doing this:

$ postgres --help
postgres is the PostgreSQL server.

Usage:
  postgres [OPTION]...

Options:
  -A 1|0 enable/disable run-time assert checking
  ...

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] libpq connection status and closed fd

[Tom Lane]

Daniele Varrazzo  writes:
> a psycopg user is reporting [1] that the library is not marking the
> connection as closed and/or bad after certain errors, such as a
> connection timeout. He is emulating the error by closing the
> connection fd

That seems like a completely illegitimate test procedure.  A network
failure does not result in automatic closure of the FD so there is
no reason for the libpq code to be expecting that to happen.  Instead,
it expects to see an appropriate error code next time it tries to
use the socket.

If you want to test for connection loss, consider doing a kill -9 on
the connected backend (best not to do that with a production server
of course).

A larger point is that your user may well have false expectations
about how quickly libpq will detect connection loss.  Generally
that won't happen until it next tries to transmit or receive some
data; so a simple PQstatus() call doesn't prove a lot about whether
the connection has been lost since the last query.  This is not a
bug either IMO.

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] Index scan optimization

[Heikki Linnakangas]

On 09/22/2014 04:45 PM, Tom Lane wrote:

Heikki Linnakangas  writes:

On 09/22/2014 07:47 AM, Rajeev rastogi wrote:

So my proposal is to skip the condition check on the first scan key condition 
for every tuple.



The same happens in a single-column case. If you have a query like
"SELECT * FROM tbl2 where id2 > 'a'", once you've found the start
position of the scan, you know that all the rows that follow match too.


... unless you're doing a backwards scan.


Sure. And you have to still check for NULLs. Have to get the details right..

- Heikki



--
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] Index scan optimization

[Tom Lane]

Heikki Linnakangas  writes:
> On 09/22/2014 07:47 AM, Rajeev rastogi wrote:
>> So my proposal is to skip the condition check on the first scan key 
>> condition for every tuple.

> The same happens in a single-column case. If you have a query like 
> "SELECT * FROM tbl2 where id2 > 'a'", once you've found the start 
> position of the scan, you know that all the rows that follow match too.

... unless you're doing a backwards scan.

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] documentation references invalid -A assertion checks option

[Andres Freund]

On 2014-09-22 08:43:02 -0500, Merlin Moncure wrote:
> Both the documentation
> (http://www.postgresql.org/docs/9.4/static/app-postgres.html) and the
> --help switch to postgres reference a -A switch to handle assertion
> checking.   Looking at the code, I don't see any entry for -A in the
> getopt string and passing -A always fails with 'invalid option'
> regardless of the compile time setting.  If I'm right, either the docs
> or the source need to be patched.  The question is, which one?  (I
> vote to remove references to the option).

You're probably building master. That option has been removed (by me)
since. The 'devel' docs don't show the option anymore unless I missed a
place referencing it.

Greetings,

Andres Freund

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


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

[HACKERS] documentation references invalid -A assertion checks option

[Merlin Moncure]

Both the documentation
(http://www.postgresql.org/docs/9.4/static/app-postgres.html) and the
--help switch to postgres reference a -A switch to handle assertion
checking.   Looking at the code, I don't see any entry for -A in the
getopt string and passing -A always fails with 'invalid option'
regardless of the compile time setting.  If I'm right, either the docs
or the source need to be patched.  The question is, which one?  (I
vote to remove references to the option).

merlin


-- 
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] Index scan optimization

[Stephen Frost]

* Rajeev rastogi (rajeev.rast...@huawei.com) wrote:
> Thanks, I shall start to prepare a patch for this optimization and share in 1 
> or 2 days.

This sounded interesting to me also- please be sure to put it on the
open commitfest once you have posted the patch.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Assertion failure in syncrep.c

[Stephen Frost]

Pavan,

* Pavan Deolasee (pavan.deola...@gmail.com) wrote:
> On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee 
> wrote:
> > While running some tests on REL9_2_STABLE branch, I saw an assertion
> > failure in syncrep.c. The stack trace looks like this:
> >
> Any comments on this? I see it very regularly during my pgbench tests.

I agree that it looks like there may be a race condition there- but
could you provide the test cases you're working with?  What kind of
behavior is it that's making it show up easily for you?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Assertion failure in syncrep.c

[Pavan Deolasee]

On Thu, Sep 18, 2014 at 12:02 PM, Pavan Deolasee 
wrote:

> Hi All,
>
> While running some tests on REL9_2_STABLE branch, I saw an assertion
> failure in syncrep.c. The stack trace looks like this:
>
>
Any comments on this? I see it very regularly during my pgbench tests.

Thanks,
Pavan

-- 
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee

Re: [HACKERS] Index scan optimization

[Rajeev rastogi]

On 22 September 2014 12:35, Heikki Linnakangas:

> > I have observed a scope of considerable performance improvement in-
> case of index by a very minor code change.
> > Consider the below schema:
> >
> > create table tbl2(id1 int, id2 varchar(10), id3 int); create index
> > idx2 on tbl2(id2, id3);
> >
> > Query as:
> >  select count(*) from tbl2 where id2>'a' and
> > id3>99;
> >
> > As per current design, it takes following steps to retrieve index
> tuples:
> >
> > 1.   Find the scan start position by searching first position in
> BTree as per the first key condition i.e. as per id2>'a'
> >
> > 2.   Then it fetches each tuples from position found in step-1.
> >
> > 3.   For each tuple, it matches all scan key condition, in our
> example it matches both scan key condition.
> >
> > 4.   If condition match, it returns the tuple otherwise scan
> stops.
> >
> > Now problem is here that already first scan key condition is matched
> to find the scan start position (Step-1), so it is obvious that any
> further tuple also will match the first scan key condition (as records
> are sorted).
> > So comparison on first scan key condition again in step-3 seems to be
> redundant.
> >
> > So my proposal is to skip the condition check on the first scan key
> condition for every tuple.
> 
> The same happens in a single-column case. If you have a query like
> "SELECT * FROM tbl2 where id2 > 'a'", once you've found the start
> position of the scan, you know that all the rows that follow match too.

Very much true.

> > I would like to submit the patch for this improvement.
> > Please provide your feedback. Also let me know if I am missing
> something.
> 
> Yeah, sounds like a good idea. This scenario might not arise very often,
> but it should be cheap to check, so I doubt it will add any measurable
> overhead to the cases where the optimization doesn't help.

Thanks, I shall start to prepare a patch for this optimization and share in 1 
or 2 days.

Thanks and Regards,
Kumar Rajeev Rastogi


-- 
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] [REVIEW] Re: Compression of full-page-writes

[Syed, Rahila]

Hello,

>Please find attached the patch to compress FPW.

Sorry I had forgotten to attach. Please find the patch attached.

Thank you,
Rahila Syed





From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Rahila Syed
Sent: Monday, September 22, 2014 3:16 PM
To: Alvaro Herrera
Cc: Rahila Syed; PostgreSQL-development; Tom Lane
Subject: Re: [HACKERS] [REVIEW] Re: Compression of full-page-writes


Hello All,


>Well, from Rahila's point of view the patch is actually attached, but
>she's posting from the Nabble interface, which mangles it and turns into
>a link instead.

Yes.



>but the end result is the
>same: to properly submit a patch, you need to send an email to the
> mailing list, not join a group/forum from
>some intermediary newsgroup site that mirrors the list.


Thank you. I will take care of it henceforth.

Please find attached the patch to compress FPW.  Patch submitted by Fujii-san 
earlier in the thread is used to merge compression GUC with full_page_writes.



I am reposting the measurement numbers.

Server Specification:
Processors:Intel® Xeon ® Processor E5-2650 (2 GHz, 8C/16T, 20 MB) * 2 nos
RAM: 32GB
Disk : HDWD 450GB 10K Hot Plug 2.5-inch SAS HDD * 8 nos
1 x 450 GB SAS HDD, 2.5-inch, 6Gb/s, 10,000 rpm

Checkpoint segments: 1024
Checkpoint timeout: 5 mins

pgbench -c 64 -j 64 -r -T 900 -M prepared
Scale factor: 1000

WAL generated (MB)   Throughput (tps)   
   Latency(ms)
On 9235.43   979.03 
  65.36
Compress(pglz)   6518.681072.34 
59.66
Off 501.04  1135.17 
   56.34

The results show  around 30 percent decrease in WAL volume due to compression 
of FPW.

Thank you ,

Rahila Syed

Tom Lane wrote:
> Rahila Syed 
> mailto:rahilasyed...@gmail.com>.90@gmail.com>
>  writes:
> > Please find attached patch to compress FPW using pglz compression.
>
> Patch not actually attached AFAICS (no, a link is not good enough).

Well, from Rahila's point of view the patch is actually attached, but
she's posting from the Nabble interface, which mangles it and turns into
a link instead.  Not her fault, really -- but the end result is the
same: to properly submit a patch, you need to send an email to the
pgsql-hackers@postgresql.orgmailing
 list, not join a group/forum from
some intermediary newsgroup site that mirrors the list.

--
Álvaro Herrera   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

__
Disclaimer:This email and any attachments are sent in strictest confidence for 
the sole use of the addressee and may contain legally privileged, confidential, 
and proprietary data.  If you are not the intended recipient, please advise the 
sender by replying promptly to this email and then delete and destroy this 
email and any attachments without any further use, copying or forwarding


compress_fpw_v1.patch
Description: compress_fpw_v1.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] [REVIEW] Re: Compression of full-page-writes

[Rahila Syed]

Hello All,


>Well, from Rahila's point of view the patch is actually attached, but
>she's posting from the Nabble interface, which mangles it and turns into
>a link instead.

Yes.



>but the end result is the
>same: to properly submit a patch, you need to send an email to the
> mailing list, not join a group/forum from
>some intermediary newsgroup site that mirrors the list.


Thank you. I will take care of it henceforth.

Please find attached the patch to compress FPW.  Patch submitted by
Fujii-san earlier in the thread is used to merge compression GUC with
full_page_writes.



I am reposting the measurement numbers.

Server Specification:
Processors:Intel® Xeon ® Processor E5-2650 (2 GHz, 8C/16T, 20 MB) * 2 nos
RAM: 32GB
Disk : HDWD 450GB 10K Hot Plug 2.5-inch SAS HDD * 8 nos
1 x 450 GB SAS HDD, 2.5-inch, 6Gb/s, 10,000 rpm

Checkpoint segments: 1024
Checkpoint timeout: 5 mins

pgbench -c 64 -j 64 -r -T 900 -M prepared
Scale factor: 1000

WAL generated (MB)   Throughput
(tps)  Latency(ms)
On 9235.43   979.03
  65.36
Compress(pglz)   6518.681072.34
59.66
Off 501.04
1135.1756.34

The results show  around 30 percent decrease in WAL volume due to
compression of FPW.

Thank you ,

Rahila Syed

Tom Lane wrote:
> Rahila Syed .90@
gmail.com > writes:
> > Please find attached patch to compress FPW using pglz compression.
>
> Patch not actually attached AFAICS (no, a link is not good enough).

Well, from Rahila's point of view the patch is actually attached, but
she's posting from the Nabble interface, which mangles it and turns into
a link instead.  Not her fault, really -- but the end result is the
same: to properly submit a patch, you need to send an email to the
pgsql - hackers
@ postgresql.org
mailing list, not join a group/forum from
some intermediary newsgroup site that mirrors the list.

--
Álvaro Herrera   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] libpq connection status and closed fd

[Marko Tiikkaja]

On 9/22/14 10:57 AM, Andres Freund wrote:

On 2014-09-22 07:42:01 +0100, Daniele Varrazzo wrote:

Is this intentional? Is there a better way to check for a broken connection?


Note that the libpq code treats connection resets differently from
other, arbitrary, errors:

I.e. if the kernel returns that the connection has been closed it'll do
different stuff.

So I'm not convinced that the testcaseq is valid. This isn't the error
you'd get after a timeout or similar. We could add a special case path
for EBADFD, but why?


Probably not for EBADFD, but how about e.g. ETIMEDOUT?  The SSL code 
path seems to be putting EPIPE into the same category as ECONNRESET, too.



.marko


--
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] libpq connection status and closed fd

[Andres Freund]

On 2014-09-22 07:42:01 +0100, Daniele Varrazzo wrote:
> Hello,
> 
> a psycopg user is reporting [1] that the library is not marking the
> connection as closed and/or bad after certain errors, such as a
> connection timeout. He is emulating the error by closing the
> connection fd (I don't know if the two conditions result in the same
> effect, but I'll stick to this hypothesis for now).
> 
> [1] https://github.com/psycopg/psycopg2/issues/263
> 
> Testing with a short C program [2] it seems that indeed, after closing
> the fd and causing an error in `PQconsumeInput`, the connection is
> deemed in good state by `PQstatus`. A similar test gives the same
> result after `PQexec` is attempted on a connection whose fd is closed
> (tests performed with PostgreSQL 9.3.5).
> 
> [2] https://gist.github.com/dvarrazzo/065f343c95f8ea67cf8f
> 
> Is this intentional? Is there a better way to check for a broken connection?

Note that the libpq code treats connection resets differently from
other, arbitrary, errors:

int
pqReadData(PGconn *conn)
{
...
nread = pqsecure_read(conn, conn->inBuffer + conn->inEnd,
  conn->inBufSize - 
conn->inEnd);
if (nread < 0)
{
if (SOCK_ERRNO == EINTR)
goto retry3;
/* Some systems return EAGAIN/EWOULDBLOCK for no data */
#ifdef EAGAIN
if (SOCK_ERRNO == EAGAIN)
return someread;
#endif
#if defined(EWOULDBLOCK) && (!defined(EAGAIN) || (EWOULDBLOCK != EAGAIN))
if (SOCK_ERRNO == EWOULDBLOCK)
return someread;
#endif
/* We might get ECONNRESET here if using TCP and backend died */
#ifdef ECONNRESET
if (SOCK_ERRNO == ECONNRESET)
goto definitelyFailed;
#endif
/* pqsecure_read set the error message for us */
return -1;
}


I.e. if the kernel returns that the connection has been closed it'll do
different stuff.

So I'm not convinced that the testcaseq is valid. This isn't the error
you'd get after a timeout or similar. We could add a special case path
for EBADFD, but why?

> If we mark the connection as closed every time `PQconsumeInput`
> returns 0 (or `PQexec` returns null, which are the two code paths
> affecting psycopg) would it be too aggressive and cause false
> positives?

Likely, yes.

Greetings,

Andres Freund

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


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

Re: [HACKERS] libpq connection status and closed fd

[Dmitriy Igrishin]

2014-09-22 12:36 GMT+04:00 Marko Tiikkaja :

> On 9/22/14 9:45 AM, Dmitriy Igrishin wrote:
>
>> 2014-09-22 11:35 GMT+04:00 Daniele Varrazzo :
>>
>>  On Mon, Sep 22, 2014 at 8:17 AM, Dmitriy Igrishin 
>>> wrote:
>>>
 Why are you using close() instead of PQfinish()?

>>>
>>> Because I'm testing for an error, please read my message and the
>>> original bug report.
>>>
>>>  I read it. You are testing for an error, but use libpq in a wrong way in
>> your test case. You must use PQfinish() to close the connection
>> and PQstatus() will works for you.
>>
>
> Perhaps you should go back and re-read it then.  The point of the test
> case is not to test connection closure; it's to test behaviour in the
> presence of network errors.

And where the network error emulation in the test case? By closing fd?
I'm sorry if I don't understand something, but really, I don't see any
problem
or incorrect behavior of *libpq*. It's behavior adequate to a test case.

>
>
>
> .marko
>



-- 
// Dmitriy.

Re: [HACKERS] libpq connection status and closed fd

[Marko Tiikkaja]

On 9/22/14 9:45 AM, Dmitriy Igrishin wrote:

2014-09-22 11:35 GMT+04:00 Daniele Varrazzo :


On Mon, Sep 22, 2014 at 8:17 AM, Dmitriy Igrishin 
wrote:

Why are you using close() instead of PQfinish()?


Because I'm testing for an error, please read my message and the
original bug report.


I read it. You are testing for an error, but use libpq in a wrong way in
your test case. You must use PQfinish() to close the connection
and PQstatus() will works for you.


Perhaps you should go back and re-read it then.  The point of the test 
case is not to test connection closure; it's to test behaviour in the 
presence of network errors.



.marko


--
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] libpq connection status and closed fd

[Dmitriy Igrishin]

2014-09-22 11:35 GMT+04:00 Daniele Varrazzo :

> On Mon, Sep 22, 2014 at 8:17 AM, Dmitriy Igrishin 
> wrote:
> >
> > 2014-09-22 10:42 GMT+04:00 Daniele Varrazzo  >:
>
> >> [2] https://gist.github.com/dvarrazzo/065f343c95f8ea67cf8f
> >
> > Why are you using close() instead of PQfinish()?
>
> Because I'm testing for an error, please read my message and the
> original bug report.
>
I read it. You are testing for an error, but use libpq in a wrong way in
your test case. You must use PQfinish() to close the connection
and PQstatus() will works for you.


> -- Daniele
>



-- 
// Dmitriy.

Re: [HACKERS] libpq connection status and closed fd

[Daniele Varrazzo]

On Mon, Sep 22, 2014 at 8:17 AM, Dmitriy Igrishin  wrote:
>
> 2014-09-22 10:42 GMT+04:00 Daniele Varrazzo :

>> [2] https://gist.github.com/dvarrazzo/065f343c95f8ea67cf8f
>
> Why are you using close() instead of PQfinish()?

Because I'm testing for an error, please read my message and the
original bug report.

-- Daniele


-- 
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] libpq connection status and closed fd

[Dmitriy Igrishin]

2014-09-22 10:42 GMT+04:00 Daniele Varrazzo :

> Hello,
>
> a psycopg user is reporting [1] that the library is not marking the
> connection as closed and/or bad after certain errors, such as a
> connection timeout. He is emulating the error by closing the
> connection fd (I don't know if the two conditions result in the same
> effect, but I'll stick to this hypothesis for now).
>
> [1] https://github.com/psycopg/psycopg2/issues/263
>
> Testing with a short C program [2] it seems that indeed, after closing
> the fd and causing an error in `PQconsumeInput`, the connection is
> deemed in good state by `PQstatus`. A similar test gives the same
> result after `PQexec` is attempted on a connection whose fd is closed
> (tests performed with PostgreSQL 9.3.5).
>
> [2] https://gist.github.com/dvarrazzo/065f343c95f8ea67cf8f

Why are you using close() instead of PQfinish()?

>
>
> Is this intentional? Is there a better way to check for a broken
> connection?
>
BTW, PQsocket() returns -1 after PQfinish().

>
> If we mark the connection as closed every time `PQconsumeInput`
> returns 0 (or `PQexec` returns null, which are the two code paths
> affecting psycopg) would it be too aggressive and cause false
> positives?
>
> Thank you very much.
>
> -- Daniele
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
// Dmitriy.

Re: [HACKERS] Index scan optimization

[Heikki Linnakangas]

On 09/22/2014 07:47 AM, Rajeev rastogi wrote:

I have observed a scope of considerable performance improvement in-case of 
index by a very minor code change.
Consider the below schema:

create table tbl2(id1 int, id2 varchar(10), id3 int);
create index idx2 on tbl2(id2, id3);

Query as:
 select count(*) from tbl2 where id2>'a' and id3>99;

As per current design, it takes following steps to retrieve index tuples:

1.   Find the scan start position by searching first position in BTree as per 
the first key condition i.e. as per id2>'a'

2.   Then it fetches each tuples from position found in step-1.

3.   For each tuple, it matches all scan key condition, in our example it 
matches both scan key condition.

4.   If condition match, it returns the tuple otherwise scan stops.

Now problem is here that already first scan key condition is matched to find 
the scan start position (Step-1), so it is obvious that any further tuple also 
will match the first scan key condition (as records are sorted).
So comparison on first scan key condition again in step-3 seems to be redundant.

So my proposal is to skip the condition check on the first scan key condition 
for every tuple.


The same happens in a single-column case. If you have a query like 
"SELECT * FROM tbl2 where id2 > 'a'", once you've found the start 
position of the scan, you know that all the rows that follow match too.



I would like to submit the patch for this improvement.
Please provide your feedback. Also let me know if I am missing something.


Yeah, sounds like a good idea. This scenario might not arise very often, 
but it should be cheap to check, so I doubt it will add any measurable 
overhead to the cases where the optimization doesn't help.


- Heikki



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