Re: [HACKERS] application_name in process name?

[Jim Nasby]

On 7/13/16 12:07 PM, Tom Lane wrote:

Mike Blackwell  writes:

There are times when it would be useful to have the application_name
connection parameter displayed in the process name - and thus in ps and
pg_top - in addition to the user and database name.



Would there be any downside to this?


In a lot of situations ("top" for instance) only a limited number of
characters can be displayed from a process title.  I'm hesitant to add
fields to that string that we don't really need.


Could we make this configurable, similar to log_line_prefix?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

Re: [HACKERS] Constraint merge and not valid status

[Jim Nasby]

On 7/13/16 4:22 AM, Amit Langote wrote:

Consider a scenario where one adds a *valid* constraint on a inheritance
parent which is then merged with a child table's *not valid* constraint
during inheritance recursion.  If merged, the constraint is not checked
for the child data even though it may have some.  Is that an oversight?


Seen as how you used to be able to illegally twerk NOT NULL status on 
children (and maybe still can), I'd bet this is a bug...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


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

[HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Noah Misch]

On Wed, Jul 13, 2016 at 03:57:02PM -0500, Kevin Grittner wrote:
> On Wed, Jul 13, 2016 at 12:48 PM, Andres Freund  wrote:
> > On 2016-07-13 10:06:52 -0500, Kevin Grittner wrote:
> >> On Wed, Jul 13, 2016 at 7:57 AM, Amit Kapila  
> >> wrote:
> >>> On Tue, Jul 12, 2016 at 8:34 PM, Kevin Grittner  wrote:
>  On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund  wrote:
> 
> > I'm a bit confused, why aren't we simply adding LSN interlock
> > checks for toast? Doesn't look that hard? Seems like a much more
> > natural course of fixing this issue?
> 
>  I took some time trying to see what you have in mind, and I'm
>  really not "getting it".
> >>>
> >>> Isn't it possible if we initialize lsn and whenTaken in SnapshotToast
> >>> when old_snapshot_threshold > 0 and add a check for
> >>> HeapTupleSatisfiesToast in TestForOldSnapshot()?
> >>
> >> With that approach, how will we know *not* to generate an error
> >> when reading the chain of tuples for a value we are deleting.  Or
> >> positioning to modify an index on toast data.  Etc., etc. etc.
> >
> > I'm not following. How is that different in the toast case than in the
> > heap case?
> 
> A short answer is that a normal table's heap doesn't go through
> systable_getnext_ordered().  That function is used both for cases
> where the check should not be made, like toast_delete_datum(), and
> cases where it should, like toast_fetch_datum().
> 
> Since this keeps coming up, I'll produce a patch this way.  I'm
> skeptical, but maybe it will look better than I think it will.  I
> should be able to post that by Friday.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com


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

[HACKERS] plperl loading files

[Jeff Janes]

After putting "plperl" into shared_preload_libraries so that things
get loaded upon server start, I was surprised to see that each backend
was still accessing a handful of perl files the first time it used a
plperl function.

I see that the cause is src/pl/plperl/plc_trusted.pl, which loads some
modules only when running plperl (not plperlu), and so it happens in
the backend rather than the postmaster.

Is there some pressing need for these module not to be loaded for use
with plperlu? I think the rationale is that plperlu can load them for
itself if it wants them, so they don't need to be loaded
automatically.  But I don't see why they need to not be loaded
automatically.  If that was changed then the loading could be moved to
the server start rather than each backend's first use of plperl(u).

If you really want to suppress this per-backend activity, you can
pre-emptively load the modules yourself with something like:

plperl.on_init ='require Carp; require Carp::Heavy; require feature;'

But, I don't see why that should be necessary.  I'd like to just put
plperl into shared_preload_libraries and be done with it.

Am I missing something here?

Cheers,

Jeff


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

Re: [HACKERS] DO with a large amount of statements get stuck with high memory consumption

[Jan Wieck]

On Tue, Jul 12, 2016 at 3:29 PM, Merlin Moncure  wrote:

> I've noticed that pl/pgsql functions/do commands do not behave well
> when the statement resolves and frees memory.   To be clear:
>
> FOR i in 1..100
> LOOP
>   INSERT INTO foo VALUES (i);
> END LOOP;
>
> ...runs just fine while
>
> BEGIN
>   INSERT INTO foo VALUES (1);
>   INSERT INTO foo VALUES (2);
>   ...
>   INSERT INTO foo VALUES (100);
> END;
>

This sounds very much like what led to
commit 25c539233044c235e97fd7c9dc600fb5f08fe065.

It seems that patch was only applied to master and never backpatched to 9.5
or earlier.


Regards, Jan




>
> (for the curious, create a script yourself via
> copy (
>   select
> 'do $$begin create temp table foo(i int);'
>   union all select
> format('insert into foo values (%s);', i) from
> generate_series(1,100) i
>   union all select 'raise notice ''abandon all hope!''; end; $$;'
> ) to '/tmp/breakit.sql';
>
> ...while consume amounts of resident memory proportional to the number
> of statemnts and eventually crash the server.  The problem is obvious;
> each statement causes a plan to get created and the server gets stuck
> in a loop where SPI_freeplan() is called repeatedly.  Everything is
> working as designed I guess, but when this happens it's really
> unpleasant: the query is uncancellable and unterminatable, nicht gut.
> A pg_ctl kill ABRT  will do the trick but I was quite astonished
> to see linux take a few minutes to clean up the mess (!) on a somewhat
> pokey virtualized server with lots of memory.  With even as little as
> ten thousand statements the cleanup time far exceed the runtime of the
> statement block.
>
> I guess the key takeaway here is, "don't do that"; pl/pgsql
> aggressively generates plans and turns out to be a poor choice for
> bulk loading because of all the plan caching.   Having said that, I
> can't help but wonder if there should be a (perhaps user configurable)
> limit to the amount of SPI plans a single function call should be able
> to acquire on the basis you are going to smack into very poor
> behaviors in the memory subsystem.
>
> Stepping back, I can't help but wonder what the value of all the plan
> caching going on is at all for statement blocks.  Loops might comprise
> a notable exception, noted.  I'd humbly submit though that (relative
> to functions) it's much more likely to want to do something like
> insert a lot of statements and a impossible to utilize any cached
> plans.
>
> This is not an academic gripe -- I just exploded production :-D.
>
> merlin
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Jan Wieck
Senior Postgres Architect

Re: [HACKERS] GiST index build versus NaN coordinates

[Tom Lane]

Andreas Seltenreich  writes:
> I wrote:
>> Sounds like some fuzz testing with nan/infinity is in order.

> related fallout: close_ps returns a NULL pointer with NaNs around:
> select close_ps('(nan,nan)', '(nan,nan),(nan,nan)');
> -- TRAP: FailedAssertion("!(result != ((void *)0))", File: "geo_ops.c", Line: 
> 2860)

Yeah, that Assert seems way too optimistic.  Even without NaNs, I wonder
whether plain old roundoff error couldn't trigger cases where interpt_sl
fails to find an intersection point.  I'm inclined to just let close_ps
return SQL NULL in such cases.

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] Reviewing freeze map code

[Andres Freund]

On July 16, 2016 8:49:06 AM PDT, Tom Lane  wrote:
>Amit Kapila  writes:
>> On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund 
>wrote:
>>> I think we have two choices how to deal with that: First, we can add
>a
>>> new flags variable to xl_heap_lock similar to
>>> xl_heap_insert/update/... and bump page magic,
>
>> +1 for going in this way.  This will keep us consistent with how
>clear
>> the visibility info in other places like heap_xlog_update().
>
>Yeah.  We've already forced a catversion bump for beta3, and I'm about
>to go fix PG_CONTROL_VERSION as well, so there's basically no downside
>to doing an xlog version bump as well.  At least, not if you can get it
>in before Monday.

OK, Cool. Will do it later today.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


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

Re: [HACKERS] Reviewing freeze map code

[Tom Lane]

Amit Kapila  writes:
> On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund  wrote:
>> I think we have two choices how to deal with that: First, we can add a
>> new flags variable to xl_heap_lock similar to
>> xl_heap_insert/update/... and bump page magic,

> +1 for going in this way.  This will keep us consistent with how clear
> the visibility info in other places like heap_xlog_update().

Yeah.  We've already forced a catversion bump for beta3, and I'm about
to go fix PG_CONTROL_VERSION as well, so there's basically no downside
to doing an xlog version bump as well.  At least, not if you can get it
in before Monday.

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] Regression tests vs existing users in an installation

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> We've talked before about how the regression tests should be circumspect
>> about what role names they create/drop, so as to avoid possibly blowing
>> up an installation's existing users during "make installcheck".  In
>> particular I believe there was consensus that such names should begin
>> with, or at least include, "regress".  I got around today to instrumenting
>> CreateRole to see what names we were actually creating, and was quite
>> depressed as to how thoroughly that guideline is being ignored (see
>> attached).

> I would propose that we have one test run near the beginning or right at
> the beginning of the serial schedule that sets up a small number of
> roles to cover most of the needs of every other test, so that most such
> other tests do not need to create any roles at all.

I don't think that's a very attractive idea.  It would create hazards for
concurrent test cases, I fear.  Moreover, an un-enforced convention of
"don't create roles" isn't really any safer than an un-enforced convention
of "only create roles named thus-and-such"; it just takes one person who
is not familiar with the convention, and one committer not paying close
attention, and we're right back where we started.

I'm coming to the conclusion that the only thing that will make this
materially better in the long run is automatic enforcement of a convention
about what role names may be created in the regression tests.  See my
response to Stephen just now for a concrete proposal.

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] Regression tests vs existing users in an installation

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> One could certainly argue that these are safe enough because nobody would
>> ever create real roles by those names anyway.  I'm not very comfortable
>> with that though; if we believe that, why did we go to the trouble of
>> making sure these cases work?

> Sadly, it's not quite so simple:
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831234

Hah.  Well, I have zero interest in supporting "user" as the name of the
bootstrap superuser.  Even if that seemed like a good idea, why not also
current_user, session_user, Public, or any other name we might want to use
in the tests?  The variant-output-files answer definitely doesn't scale.

What seems a more useful approach is for packagers to not allow the O/S
username to affect the results of "make check".  initdb already has the
--username switch to override its choice of the bootstrap superuser name,
and pg_regress has a --user switch, so in principle it should be possible
for a package to ensure that its build tests are run with the standard
superuser name rather than something dependent on the environment.  I'm
not sure how *easy* it is, mind you.  We might want to add some Makefile
plumbing to make it easier to do that.

But that's not what I'm on about today ...

>> A more aggressive answer would be to decide we don't need these test cases
>> at all and drop them.  An advantage of that is that then we could
>> configure some buildfarm animal to fail the next time somebody ignores
>> the "test role names should contain 'regress'" rule.

> I'd really like to have more test coverage rather than less, so I'd find
> this a bit hard to swallow, but it might still be better than the
> alternatives.

As Greg mentioned, we could improve things if we were willing to invent
something like a "regression_test_mode" GUC.  The rough sketch would be:

1. pg_regress adds "regression_test_mode = on" to the ALTER DATABASE SET
settings it already applies to created databases.

2. One of the effects of the GUC would be to throw an error (or warning?)
for creation of disallowed-by-convention role names.

3. The rolenames test could turn off the GUC during creation of these
specific test names.  Or if we make it report WARNING not ERROR, we don't
even have to do that, just include the warnings in the expected output.

I find myself liking this idea, because there would be other uses for such
a GUC.  One thing that is awful darn tempting right now is to get rid of
the "force_parallel_mode = regress" wart, making that variable back into
a plain bool, and instead have that behavior emerge from having both
force_parallel_mode and regression_test_mode turned on.

We'd still want creation of these specific role names to be wrapped in
a rolled-back transaction, so that we could document that only role
names beginning with "regress_" are at hazard from "make installcheck".
But I don't think that doing that really represents any meaningful loss
of coverage.

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] Regression tests vs existing users in an installation

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> We've talked before about how the regression tests should be circumspect
> about what role names they create/drop, so as to avoid possibly blowing
> up an installation's existing users during "make installcheck".  In
> particular I believe there was consensus that such names should begin
> with, or at least include, "regress".  I got around today to instrumenting
> CreateRole to see what names we were actually creating, and was quite
> depressed as to how thoroughly that guideline is being ignored (see
> attached).

I had started in on this but hadn't made as much progress as I had
hoped, so I'm glad to see that you're taking a look at it.

> I propose to go through the regression tests and fix this (in HEAD only).
> However, there's one place where it's not so easy to just substitute a
> different name, because rolenames.sql is intentionally doing this:
> 
> CREATE ROLE "Public";
> CREATE ROLE "None";
> CREATE ROLE "current_user";
> CREATE ROLE "session_user";
> CREATE ROLE "user";
> 
> in order to test whether we properly distinguish role-related keywords
> from quoted identifiers.  Obviously, modifying these would defeat the
> point of the test.
> 
> One could certainly argue that these are safe enough because nobody would
> ever create real roles by those names anyway.  I'm not very comfortable
> with that though; if we believe that, why did we go to the trouble of
> making sure these cases work?

Sadly, it's not quite so simple:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=831234

Note that Christoph went ahead and closed out the bug report as it was
just an issue in the regression test and not unexpected behavior, but if
we're going to do something in this area then we may wish to consider
fixing the case that caused that bug report.

> What I'm inclined to do with this is to reduce the test to be something
> like
> 
> BEGIN;
> CREATE ROLE "Public";
> CREATE ROLE "None";
> CREATE ROLE "current_user";
> CREATE ROLE "session_user";
> CREATE ROLE "user";
> ROLLBACK;
> 
> with maybe a couple of ALTERs and GRANTs inside the transaction to verify
> that the names can be referenced as well as created.  This would be safe
> against modifying any conflicting existing users; the only bad consequence
> would be a phony failure of the test.

Unfortunately, the above wouldn't fix the case where someone attempts to
run the regression tests as a Unix user named "user".

One suggestion discussed on the bug report was to include different
result files, but that does seem pretty special-cased and overkill,
though if the overall set of tests in rolenames.sql is reduced then
perhaps it isn't as much of an issue.  Then again, how many different
result files would be needed to cover all cases?  Seems like there could
be quite a few, though, with this specific case, it looks like we'd at
least only have to deal with one for each role which is allowed to exist
already (such as "user"), without any multiplicative factors (can't run
the regression test as more than one Unix user at a time).

> A more aggressive answer would be to decide we don't need these test cases
> at all and drop them.  An advantage of that is that then we could
> configure some buildfarm animal to fail the next time somebody ignores
> the "test role names should contain 'regress'" rule.

I'd really like to have more test coverage rather than less, so I'd find
this a bit hard to swallow, but it might still be better than the
alternatives.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] GiST index build versus NaN coordinates

[Andreas Seltenreich]

I wrote:

> Sounds like some fuzz testing with nan/infinity is in order.

related fallout: close_ps returns a NULL pointer with NaNs around:

select close_ps('(nan,nan)', '(nan,nan),(nan,nan)');
-- TRAP: FailedAssertion("!(result != ((void *)0))", File: "geo_ops.c", Line: 
2860)

regards,
Andreas


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

Re: [HACKERS] Regression tests vs existing users in an installation

[Greg Stark]

On 16 Jul 2016 12:59 pm, "Michael Paquier" 
wrote:
>

> Thanks for doing this.

+1

Though I might highlight this as the kind of issue that a bug tracker would
help avoid falling through the cracks and make visible to newcomers.

> I am -1 for dropping the tests. We could just have a CFLAGS that adds
> an elog(ERROR) in CreateRole and checks that the created role has a
> wanted prefix, or have a plugin that uses the utility hook to do this
> filtering.

If we make a hidden regression_test_safety GUC then we could have
pg_regress enable it and have these specific tests disable it explicitly
with comments on why it's safe.

It might even be handy for other people writing application regression
tests depending on what other things it blocked.

A hook might even be possible to use the same way. pg_regress would have to
build and install a .so which might be tricky.

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Amit Kapila]

On Wed, Jul 13, 2016 at 8:56 AM, Michael Paquier
 wrote:
> On Tue, Jul 12, 2016 at 12:22 PM, Amit Kapila  wrote:
>> I think updating minRecoveryPoint unconditionally can change it's
>> purpose in some cases.  Refer below comments in code:
>>
>> * minRecoveryPoint is updated to the latest replayed LSN whenever we
>> * flush a data change during archive recovery. That guards against
>> * starting archive recovery, aborting it, and restarting with an earlier
>> * stop location. If we've already flushed data changes from WAL record X
>> * to disk, we mustn't start up until we reach X again.
>>
>> Now, as per above rule, the value of minRecoveryPoint can be much
>> smaller than XLogCtl->replayEndRecPtr.  I think this can change the
>> rules when we can allow read-only connections.
>
> That would delay the moment read-only connections in hot standby are
> allowed in the case where a server is stopped with "immediate", then
> restarted with a different target if no data has been flushed when
> replaying.
>
>> I think your and Kyotaro-san's point that minRecoveryPoint should be
>> updated to support back-ups on standby has merits, but I think doing
>> it unconditionally might lead to change in behaviour in some cases.
>
> If we want to tackle the case I mentioned above, one way is to just
> update minRecoveryPoint when an exclusive or a non-exclusive backup is
> being taken by looking at their status in shared memory. See for
> example the patch (1) attached, but this does not save from the case
> where a node replays WAL, does not have data flushes, and from which a
> backup is taken, in the case where this node gets restarted later with
> the immediate mode and has different replay targets.
>

This looks clumsy as it updates minrecoverypoint for a specific
condition and it doesn't solve the above mentioned inconcistency.

> Another way that just popped into my mind is to add dedicated fields
> to XLogCtl that set the stop LSN of a backup the way it should be
> instead of using minRecoveryPoint. In short we'd update those fields
> in CreateRestartPoint and UpdateMinRecoveryPoint under
> XLogCtl->info_lck. The good thing is that this lock is already taken
> there. See patch (2) accomplishing that.
>

How is it different/preferable then directly using
XLogCtl->replayEndRecPtr and XLogCtl->replayEndTLI for stop backup
purpose?  Do you see any problem if we go with what Kyotaro-san has
proposed in the initial patch [1] (aka using
XLogCtl->lastReplayedEndRecPtr and XLogCtl->lastReplayedTLI as stop
backup location)?


[1] - 
https://www.postgresql.org/message-id/20160609.215558.118976703.horiguchi.kyotaro%40lab.ntt.co.jp


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


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

Re: [HACKERS] Regression tests vs existing users in an installation

[Michael Paquier]

On Sat, Jul 16, 2016 at 7:13 AM, Tom Lane  wrote:
> We've talked before about how the regression tests should be circumspect
> about what role names they create/drop, so as to avoid possibly blowing
> up an installation's existing users during "make installcheck".  In
> particular I believe there was consensus that such names should begin
> with, or at least include, "regress".  I got around today to instrumenting
> CreateRole to see what names we were actually creating, and was quite
> depressed as to how thoroughly that guideline is being ignored (see
> attached).

Thanks for doing this.

> A more aggressive answer would be to decide we don't need these test cases
> at all and drop them.  An advantage of that is that then we could
> configure some buildfarm animal to fail the next time somebody ignores
> the "test role names should contain 'regress'" rule.

I am -1 for dropping the tests. We could just have a CFLAGS that adds
an elog(ERROR) in CreateRole and checks that the created role has a
wanted prefix, or have a plugin that uses the utility hook to do this
filtering.
-- 
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] visibilitymap_clear()s in vacuumlazy.c aren't WAL logged

[Michael Paquier]

On Sat, Jul 16, 2016 at 10:23 AM, Andres Freund  wrote:
> The $subject says it all. Am I missing something, or is that not ok?

Indeed, it would be a good thing to get those sanity checks logged so
as standbys get the call, or at least perform the sanity check as well
on the block that the system is warning about. It seems to me that
logging the VM bit would be an interesting option as well, bringing to
the fact that there should be dedicated WAL records.

(By the way, adding a check for the case where a page contains
non-frozen tuples but the VM has the all-frozen bit set would be good
as well)

> Now, these branches should never be hit, but it surely isn't good that
> the corruption will persist on a primary, after it's explicitly been
> fixed on the standby.

"primary" and "standby" are reversed here?
-- 
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] Reviewing freeze map code

[Amit Kapila]

On Sat, Jul 16, 2016 at 7:02 AM, Andres Freund  wrote:
> On 2016-07-13 23:06:07 -0700, Andres Freund wrote:
>> > +   /* Clear only the all-frozen bit on visibility map if needed */
>> > +   if (PageIsAllVisible(BufferGetPage(buffer)) &&
>> > +   VM_ALL_FROZEN(relation, block, ))
>> > +   {
>> > +   visibilitymap_clear_extended(relation, block, vmbuffer,
>> > +  
>> >   VISIBILITYMAP_ALL_FROZEN);
>> > +   }
>> > +
>>
>> FWIW, I don't think it's worth introducing visibilitymap_clear_extended.
>> As this is a 9.6 only patch, i think it's better to change
>> visibilitymap_clear's API.
>
> Besides that easily fixed issue, the code also has the significant issue
> that it's only performing the the visibilitymap processing in the
> BLK_NEEDS_REDO case. But that's not ok, because both in the BLK_RESTORED
> and the BLK_DONE cases the visibilitymap isn't guaranteed (or even
> likely in the former case) to have been updated.
>
> I think we have two choices how to deal with that: First, we can add a
> new flags variable to xl_heap_lock similar to
> xl_heap_insert/update/... and bump page magic,
>

+1 for going in this way.  This will keep us consistent with how clear
the visibility info in other places like heap_xlog_update().



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


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

Re: [HACKERS] raw output from copy

[Pavel Stehule]

Hi

2016-04-05 10:45 GMT+02:00 Pavel Stehule :

> Hi
>
> here is cleaned/finished previous implementation of RAW_TEXT/RAW_BINARY
> formats for COPY statements.
>
> The RAW with text formats means unescaped data, but with correct encoding
> - input/output is realised with input/output function. RAW binary means
> content produced/received by sending/received functions.
>
> Now both directions (input/output) working well
>
> Some examples of expected usage:
>
> copy (select xmlelement(name foo, 'hello')) to stdout (format raw_binary,
> encoding 'latin2');
>
> create table avatars(id serial, picture bytea);
> \copy avatars(picture) from ~/images/foo.jpg (format raw_binary);
> select lastval();
>
> create table doc(id serial, txt text);
> \copy doc(txt) from ~/files/aaa.txt (format raw_text, encoding 'latin2');
> select lastval();
>
> Regards
>
> Pavel
>
>
I am sending fresh version of COPY RAW patch.

There is new regress client test requested by Tom.

Note: I though about another solution based on binary parameters and binary
result support in psql. Somelike:

INSERT INTO foo(a) VALUES($1)
\gpush filename

SELECT a FROM foo
\gpop filename

but, it is less intuitive, and doesn't work with stdin/stdout - so it is
significant week against COPY based solution for scripting from shell. More
\g* solution is still possible if will be requested in future.

Regards

Pavel

[pavel@nemesis ~]$ cat avatar.gif | psql -Xq -At -c "copy xx(b) from stdin
(format raw_text)" -c "select lastval()" postgres
313
commit d62c1ff8dee2324ce1fe7765c2d015e68f5f923a
Author: Pavel Stehule 
Date:   Sat Jul 16 10:35:25 2016 +0200

with regress tests

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 6285dd0..4c6cacb 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3226,8 +3226,9 @@ int PQfformat(const PGresult *res,
 
   
Format code zero indicates textual data representation, while format
-   code one indicates binary representation.  (Other codes are reserved
-   for future definition.)
+   code one indicates binary representation. Format code two indicates
+   raw_text representation and format code three indicates raw_binary
+   representation (Other codes are reserved for future definition.)
   
  
 
@@ -3557,6 +3558,26 @@ typedef struct

 

+
+ 
+  PQcopyFormat
+  
+   PQcopyFormat
+  
+ 
+
+ 
+  
+   Format code zero indicates textual data representation, format one
+   indicates binary representation, format two indicates raw
+   representation.
+
+int PQcopyFormat(PGresult *res);
+
+  
+ 
+
+
 
  
   PQcmdStatus
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9c96d8f..adcff46 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -3239,6 +3239,7 @@ CopyInResponse (B)
 characters, etc).
 1 indicates the overall copy format is binary (similar
 to DataRow format).
+2 indicates the overall copy format is raw.
 See 
 for more information.
 
@@ -3262,8 +3263,9 @@ CopyInResponse (B)
 
 
 The format codes to be used for each column.
-Each must presently be zero (text) or one (binary).
-All must be zero if the overall copy format is textual.
+Each must be zero (text), one (binary), two (raw_text)
+or three (raw_binary). All must be zero if the overall
+copy format is textual.
 
 
 
@@ -3313,7 +3315,8 @@ CopyOutResponse (B)
 is textual (rows separated by newlines, columns
 separated by separator characters, etc). 1 indicates
 the overall copy format is binary (similar to DataRow
-format). See  for more information.
+format). 2 indicates raw_text or raw_binary format.
+See  for more information.
 
 
 
@@ -3335,8 +3338,9 @@ CopyOutResponse (B)
 
 
 The format codes to be used for each column.
-Each must presently be zero (text) or one (binary).
-All must be zero if the overall copy format is textual.
+Each must be zero (text), one (binary), two (raw_text)
+or three (raw_binary). All must be zero if the overall
+copy format is textual.
 
 
 
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 07e2f45..4e339e4 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -197,7 +197,9 @@ COPY { table_name [ ( text,
   csv (Comma Separated Values),
-  or binary.
+  binary,
+  raw_text
+  or raw_binary.
   The default is text.
  
 
@@ -888,6 +890,44 @@ OIDs to be shown as null if that ever proves desirable.
 

   
+
+  
+