Re: Consistently use "startup process"/"WAL sender" instead of "Startup process"/"WAL Sender" in comments and docs.

[John Naylor]

On Mon, Feb 14, 2022 at 11:12 AM Bharath Rupireddy
 wrote:

> > FWIW, docs need to hold to a higher standard than code comments.
>
> Thanks. In general, I agree that the docs and error/log messages
> (user-facing) need to be of higher standard, but at the same time code
> comments too IMHO. Because many hackers/developers consider code
> comments a great place to learn the internals, being consistent there
> does no harm.

git log and git blame are more useful when they are free of minor
stylistic changes. Of course, corrections and clarity are different
matters that are worth fixing in the comments. I've pushed the doc
fixes, thanks for the patch!


--
John Naylor
EDB: http://www.enterprisedb.com

Re: Time to increase hash_mem_multiplier default?

[Peter Geoghegan]

On Wed, Jan 19, 2022 at 11:32 AM John Naylor
 wrote:
> I don't have anything really profound to say here, but in the last
> year I did on a couple occasions recommend clients to raise
> hash_mem_multiplier to 2.0 to fix performance problems.

I would like to push ahead with an increase in the default for
Postgres 15, to 2.0.

Any objections to that plan?

-- 
Peter Geoghegan

Re: [PATCH] Accept IP addresses in server certificate SANs

[Kyotaro Horiguchi]

(This needs rebasing)

At Wed, 9 Feb 2022 00:52:48 +, Jacob Champion  wrote 
in 
> On Mon, 2022-02-07 at 17:29 +0900, Kyotaro Horiguchi wrote:
> > I feel this should be a part of 0001.  (But the patches will be
> > finally merged so maybe no need to bother moving it).
> 
> Okay. I can move it easily if you feel like it would help review, but
> for now I've kept it in 0002.

Thanks.

> > So, inet_aton there seems to be the right choice but the comment
> > doesn't describe the reason for that behavior. I think we should add
> > an explanation about the reason for the behavior, maybe something like
> > this:
> > 
> > > We accept alternative IPv4 address notations that are accepted by
> > > inet_aton but not by inet_pton as server address.
> 
> I've pulled this wording into the comment in v5, attached.

> > +   if (name->type == host_type)
> > +   check_cn = false;
> > 
> > Don't we want a concise coment for this?
> 
> Added one; see what you think.

That's fine with me.

> > >   if (rc != 0)
> > > + {
> > > + /*
> > > +  * don't fall back to CN when we have a 
> > > match or have an error
> > > +  */
> > > + check_cn = false;
> > >   break;
> > > + }
> > ...
> > > - if ((rc == 0) && check_cn)
> > > + if (check_cn)
> 
> If I understand right, that's not quite equivalent (and the new tests
> fail if I implement it that way). We have to disable fallback if the
> SAN exists, whether it matches or not.

# I forgot to mention that, the test fails for me even without the
# change.  I didn't checked what is wrong there, though.

Mmm. after the end of the loop, rc is non-zero only when the loop has
exited by the break and otherwise rc is zero. Why isn't it equivalent
to setting check_cn to false at the break?

Anyway, apart from that detail, I reconfirmed the spec the patch is
going to implement.

  * If connhost contains a DNS name, and the certificate's SANs contain any
  * dNSName entries, then we'll ignore the Subject Common Name entirely;
  * otherwise, we fall back to checking the CN. (This behavior matches the
  * RFC.)

Sure.

  * If connhost contains an IP address, and the SANs contain iPAddress
  * entries, we again ignore the CN. Otherwise, we allow the CN to match,
  * EVEN IF there is a dNSName in the SANs. (RFC 6125 prohibits this: "A
  * client MUST NOT seek a match for a reference identifier of CN-ID if the
  * presented identifiers include a DNS-ID, SRV-ID, URI-ID, or any
  * application-specific identifier types supported by the client.")

Actually the patch searches for a match of IP address connhost from
dNSName SANs even if iPAddress SANs exist.  I think we've not
explicitly defined thebehavior in that case.  I supposed that we only
be deviant in the case "IP address connhost and no SANs of any type
exists". What do you think about it?

- For the certificate that have only dNSNames or no SANs presented, we
  serach for a match from all dNSNames if any or otherwise try CN
  regardless of the type of connhost.

- Otherwise (the cert has at least one iPAddress SANs) we follow the RFCs.

  - For IP-addr connhost, we search only the iPAddress SANs.

  - For DNSName connhost, we search only dNSName SANs if any or
otherwise try CN.

Honestly I didn't consider to that detail. On second thought, with
this specification we cannot decide the behavior unless we scanned all
SANs.  Maybe we can find an elegant implement but I don't think people
here would welcome even that level of complexity needed only for that
dubious existing use case.

What do you think about this?  And I'd like to hear from others.

> Great catch, thanks! I implemented option 2 to start. Option 1 might
> make things difficult to debug if you're connecting to a server by IP
> address but its certificate only has DNS names.

Looks fine. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: do only critical work during single-user vacuum?

[John Naylor]

On Tue, Feb 15, 2022 at 11:22 AM Peter Geoghegan  wrote:
>
> On Mon, Feb 14, 2022 at 8:04 PM John Naylor
>  wrote:
> > The failsafe mode does disable truncation as of v14:
> >
> > commit 60f1f09ff44308667ef6c72fbafd68235e55ae27
> > Author: Peter Geoghegan 
> > Date:   Tue Apr 13 12:58:31 2021 -0700
> >
> > Don't truncate heap when VACUUM's failsafe is in effect.
>
> That's true, but bear in mind that it only does so when the specific
> table being vacuumed actually triggers the failsafe. I believe that
> VACUUM(EMERGENCY) doesn't just limit itself to vacuuming tables where
> this is guaranteed (or even likely). If I'm not mistaken, it's
> possible (even likely) that there will be a table whose
> age(relfrozenxid) is high enough for VACUUM(EMERGENCY) to target the
> table, and yet not so high that the failsafe will kick in at the
> earliest opportunity.

Well, the point of inventing this new vacuum mode was because I
thought that upon reaching xidStopLimit, we couldn't issue commands,
period, under the postmaster. If it was easier to get a test instance
to xidStopLimit, I certainly would have discovered this sooner. When
Andres wondered about getting away from single user mode, I assumed
that would involve getting into areas too deep to tackle for v15. As
Robert pointed out, lazy_truncate_heap is the only thing that can't
happen for vacuum at this point, and fully explains why in versions <
14 our client's attempts to vacuum resulted in error. Since the
failsafe mode turns off truncation, vacuum should now *just work* near
wraparound. If there is any doubt, we can tighten the check for
entering failsafe.

Now, it's certainly possible that autovacuum is either not working at
all because of something broken, or is not working on the oldest
tables at the moment, so one thing we could do is to make VACUUM [with
no tables listed] get the tables from pg_class in reverse order of
max(xid age, mxid age). That way, the horizon will eventually pull
back over time and the admin can optionally cancel the vacuum at some
point. Since the order is harmless when it's not needed, we can do
that unconditionally.
-- 
John Naylor
EDB: http://www.enterprisedb.com

RE: Optionally automatically disable logical replication subscriptions on error

[osumi.takami...@fujitsu.com]

On Monday, February 14, 2022 8:58 PM Amit Kapila  
wrote:
> On Thu, Jan 6, 2022 at 11:23 AM osumi.takami...@fujitsu.com
>  wrote:
> >
> > Kindly have a look at the attached v16.
> >
> 
> Few comments:
Hi, thank you for checking the patch !

> =
> 1.
> @@ -3594,13 +3698,29 @@ ApplyWorkerMain(Datum main_arg)
> apply_error_callback_arg.command,
> apply_error_callback_arg.remote_xid,
> errdata->message);
> - MemoryContextSwitchTo(ecxt);
> +
> + if (!MySubscription->disableonerr)
> + {
> + /*
> + * Some work in error recovery work is done. Switch to the old
> + * memory context and rethrow.
> + */
> + MemoryContextSwitchTo(ecxt);
> + PG_RE_THROW();
> + }
>   }
> + else if (!MySubscription->disableonerr) PG_RE_THROW();
> 
> - PG_RE_THROW();
> 
> Can't we combine these two different checks for
> 'MySubscription->disableonerr' if you do it as a separate if check after 
> sending
> the stats message?
No, we can't. The second check of MySubscription->disableonerr is for the case
apply_error_callback_arg.command equals 0. We disable the subscription
on any errors. In other words, we need to rethrow the error in the case,
if the flag disableonerr is not set to true.

So, moving it to after sending
the stats message can't be done. At the same time, if we move
the disableonerr flag check outside of the apply_error_callback_arg.command 
condition
branch, we need to write another call of pgstat_report_subworker_error, with the
same arguments that we have now. This wouldn't be preferrable as well.

> 
> 2. Can we move the code related to tablesync worker and its error handing (the
> code insider if (am_tablesync_worker())) to a separate function say
> LogicalRepHandleTableSync() or something like that.
> 
> 3. Similarly, we can move apply-loop related code ("Run the main
> loop.") to a separate function say LogicalRepHandleApplyMessages().
> 
> If we do (2) and (3), I think the code in ApplyWorkerMain will look better. 
> What
> do you think?
I agree with (2) and (3), since those contribute to better readability.

Attached a new patch v17 that addresses those refactorings.


Best Regards,
Takamichi Osumi



v17-0001-Optionally-disable-subscriptions-on-error.patch
Description: v17-0001-Optionally-disable-subscriptions-on-error.patch

RE: Logical replication timeout problem

[wangw.f...@fujitsu.com]

On Tues, Feb 08, 2022 at 17:18 PM Kuroda, Hayato  
wrote:
> I applied your patch and confirmed that codes passed regression test.
> I put a short reviewing:
Thanks for your test and review.

> ```
> + static int skipped_changes_count = 0;
> + /*
> +  * Conservatively, at least 150,000 changes can be skipped in 1s.
> +  *
> +  * Because we use half of wal_sender_timeout as the threshold, and
> the unit
> +  * of wal_sender_timeout in process is ms, the final threshold is
> +  * wal_sender_timeout * 75.
> +  */
> + int skipped_changes_threshold = wal_sender_timeout * 75;
> ```
> 
> I'm not sure but could you tell me the background of this calculation?
> Is this assumption reasonable?
According to our discussion, we need to send keepalive messages to subscriber
when skipping changes.
One approach is that **for each skipped change**, we try to send keepalive
message by calculating whether a timeout will occur based on the current time
and the last time the keepalive was sent. But this will brings slight overhead.
So I want to try another approach: after **constantly skipping some changes**,
we try to send keepalive message by calculating whether a timeout will occur
based on the current time and the last time the keepalive was sent.

IMO, we should send keepalive message after skipping a certain number of
changes constantly.
And I want to calculate the threshold dynamically by using a fixed value to
avoid adding too much code.
In addition, different users have different machine performance, and users can
modify wal_sender_timeout, so the threshold should be dynamically calculated
according to wal_sender_timeout.

Based on these, I have tested on machines with different configurations. I took
the test results on the machine with the lowest configuration.
[results]
The number of changes that can be skipped per second : 537087 (Average)
To be safe, I set the value to 15.
(wal_sender_timeout / 2 / 1000 * 15 = wal_sender_timeout * 75)

The spec of the test server to get the threshold is:
CPU information : Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz
Memert information : 816188 kB

> ```
> @@ -654,20 +663,62 @@ pgoutput_change(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
>   {
>   case REORDER_BUFFER_CHANGE_INSERT:
>   if (!relentry->pubactions.pubinsert)
> + {
> + if (++skipped_changes_count >=
> skipped_changes_threshold)
> + {
> + OutputPluginUpdateProgress(ctx, true);
> +
> + /*
> +  * After sending keepalive message,
> reset
> +  * skipped_changes_count.
> +  */
> + skipped_changes_count = 0;
> + }
>   return;
> + }
>   break;
> ```
> 
> Is the if-statement needed? In the walsender case
> OutputPluginUpdateProgress() leads WalSndUpdateProgress(), and the
> function also has the threshold for ping-ing.
As mentioned above, we need to skip some changes continuously before
calculating whether it will time out.
If there is no if-statement here, every time a change is skipped, the timeout
will be checked. This brings extra overhead.

> ```
> static void
> -WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn,
> TransactionId xid)
> +WalSndUpdateProgress(LogicalDecodingContext *ctx, XLogRecPtr lsn,
> +TransactionId xid, bool send_keep_alive)
>  {
> - static TimestampTz sendTime = 0;
> + static TimestampTz trackTime = 0;
>   TimestampTz now = GetCurrentTimestamp();
> 
> + if (send_keep_alive)
> + {
> + /*
> +  * If half of wal_sender_timeout has lapsed without send
> message standby,
> +  * send a keep-alive message to the standby.
> +  */
> + static TimestampTz sendTime = 0;
> + TimestampTz ping_time =
> TimestampTzPlusMilliseconds(sendTime,
> +
>   wal_sender_timeout / 2);
> + if (now >= ping_time)
> + {
> + WalSndKeepalive(false);
> +
> + /* Try to flush pending output to the client */
> + if (pq_flush_if_writable() != 0)
> + WalSndShutdown();
> + sendTime = now;
> + }
> + }
> +
> ```
> 
> * +1 about renaming to trackTime.
> * `/2` might be magic number. How about following? Renaming is very welcome:
> 
> ```
> +#define WALSND_LOGICAL_PING_FACTOR 0.5
> +   static TimestampTz sendTime = 0;
> +   TimestampTz ping_time = TimestampTzPlusMilliseconds(sendTime,
> +
> +wal_sender_timeout * WALSND_LOGICAL_PING_FACTOR)
> ```
In the existing code, similar operations on 

Re: Design of pg_stat_subscription_workers vs pgstats

[Masahiko Sawada]

On Thu, Feb 3, 2022 at 2:35 PM Masahiko Sawada  wrote:
>
> On Thu, Feb 3, 2022 at 1:48 PM David G. Johnston
>  wrote:
> >
> > On Wednesday, February 2, 2022, Masahiko Sawada  
> > wrote:
> >>
> >> and have other error
> >> information in pg_stat_subscription_workers view.
> >
> >
> > What benefit is there to keeping the existing collector-based 
> > pg_stat_subscripiton_workers view?  If we re-write it using shmem IPC then 
> > we might as well put everything there and forego using a catalog.  Then it 
> > behaves in a similar manner to pg_stat_activity but for logical replication 
> > workers.
>
> Yes, but if we use shmem IPC, we need to allocate shared memory for
> them based on the number of subscriptions, not logical replication
> workers (i.e., max_logical_replication_workers). So we cannot estimate
> memory in the beginning. Also, IIUC the number of subscriptions that
> are concurrently working is limited by max_replication_slots (see
> ReplicationStateCtl) but I think we need to remember the state of
> disabled subscriptions too.

Correction; the replication state remains even after the subscription
is disabled, and it's removed only when the subscription is dropped.
Therefore, the number of subscriptions that can be active in the
database cluster is effectively limited by max_replication_slots.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: Replication slot drop message is sent after pgstats shutdown.

[Masahiko Sawada]

On Tue, Feb 15, 2022 at 12:09 PM Kyotaro Horiguchi
 wrote:
>
> At Mon, 14 Feb 2022 17:20:16 -0800, Andres Freund  wrote 
> in
> > Hi,
> >
> > On 2021-12-22 22:34:45 +0900, Masahiko Sawada wrote:
> > > I've attached an updated patch. Please review it.
> >
> > Sorry for dropping the ball on this again :(. I've pushed the fix with some
> > very minor polishing.

Thanks!

>
> > > > The attached detects that bug, but I'm not sure it's worth expending
> > > > test time, or this might be in the server test suit.
> > >
> > > Thanks. It's convenient to test this issue but I'm also not sure it's
> > > worth adding to the test suit.
> >
> > I think it's definitely worth adding a test, but I don't particularly like 
> > the
> > specific test implementation. Primarily because I think it's better to test
> > this in a cluster that stays running, so that we can verify that the slot 
> > drop
> > worked. It also doesn't seem necessary to create a separate cluster.
>
>  FWIW I agree to the proposed test on the direction.

+1

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

Re: pgsql: Add suport for server-side LZ4 base backup compression.

[Michael Paquier]

On Mon, Feb 14, 2022 at 10:53:04AM +0900, Michael Paquier wrote:
> A last thing, that has been mentioned by Andres upthread, is that we
> should be able to remove the extra commands run with --version in the
> tests of pg_basebackup, as of the attached.  I have not done that yet,
> as it seems better to wait for copperhead first, and the tests of
> pg_basebackup run before pg_verifybackup so I don't want to mask one
> error for another in case the buildfarm says something.

copperhead has reported back a couple of hours ago, and it has
switched back to green.  Hence, I have moved on with the remaining
piece and removed all those --version checks from the tests of
pg_basebackup and pg_receivewal.
--
Michael


signature.asc
Description: PGP signature

Re: Fix overflow in justify_interval related functions

[Nathan Bossart]

On Mon, Feb 14, 2022 at 08:35:43PM -0500, Joseph Koshakow wrote:
> Good catch, I didn't think about that. Though if you are pre-justifying
> the days, then I don't think it's possible for the second addition to
> days to overflow. The maximum amount the days field could be after
> the first justification is 29. I think we can simplify it further to the
> attached patch.

LGTM.  I've marked this one as ready-for-committer.  It's a little weird
that justify_hours() and justify_days() can overflow in cases where there
is still a valid interval representation, but as Tom noted, those functions
have specific charters to follow.

> + ereport(ERROR,
> + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
> + errmsg("interval out of range")));

nitpick: I think there is ordinarily an extra space before errmsg() so that
it lines up with errcode().

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: do only critical work during single-user vacuum?

[Peter Geoghegan]

On Mon, Feb 14, 2022 at 8:04 PM John Naylor
 wrote:
> The failsafe mode does disable truncation as of v14:
>
> commit 60f1f09ff44308667ef6c72fbafd68235e55ae27
> Author: Peter Geoghegan 
> Date:   Tue Apr 13 12:58:31 2021 -0700
>
> Don't truncate heap when VACUUM's failsafe is in effect.

That's true, but bear in mind that it only does so when the specific
table being vacuumed actually triggers the failsafe. I believe that
VACUUM(EMERGENCY) doesn't just limit itself to vacuuming tables where
this is guaranteed (or even likely). If I'm not mistaken, it's
possible (even likely) that there will be a table whose
age(relfrozenxid) is high enough for VACUUM(EMERGENCY) to target the
table, and yet not so high that the failsafe will kick in at the
earliest opportunity.

> To demonstrate to myself, I tried a few vacuums in a debugger session
> with a breakpoint at GetNewTransactionId(). I've only seen it reach
> here when heap truncation happens (or the not relevant for wraparound
> situations FULL and ANALYZE).

It's possible for a manually issued VACUUM to directly disable
truncation (same with index_cleanup). Without getting into the
question of what the ideal behavior might be right now, I can say for
sure that it wouldn't be difficult to teach VACUUM(EMERGENCY) to pass
down the same options.

The failsafe is essentially a mechanism that dynamically changes these
options for an ongoing vacuum, once age(relfrozenxid) crosses a
certain threshold. There is nothing fundamentally special about that.

-- 
Peter Geoghegan

Re: do only critical work during single-user vacuum?

[John Naylor]

On Fri, Feb 4, 2022 at 4:58 AM Robert Haas  wrote:

> As I said
> before, I know TRUNCATE has been an issue in the past, and if that's
> not already fixed in v14, we should. If there's other stuff, we should
> fix that too.

The failsafe mode does disable truncation as of v14:

commit 60f1f09ff44308667ef6c72fbafd68235e55ae27
Author: Peter Geoghegan 
Date:   Tue Apr 13 12:58:31 2021 -0700

Don't truncate heap when VACUUM's failsafe is in effect.
--

To demonstrate to myself, I tried a few vacuums in a debugger session
with a breakpoint at GetNewTransactionId(). I've only seen it reach
here when heap truncation happens (or the not relevant for wraparound
situations FULL and ANALYZE).

With the maximum allowable setting of autovacuum_freeze_max_age of 2
billion, the highest allowable vacuum_failsafe_age is 2.1 billion, so
heap truncation will be shut off before the warnings start.

> And then we should KILL WITH FIRE
> the message telling people to use single user mode -- and once we do
> that, the question of what the behavior ought to be when someone does
> run VACUUM in single user mode becomes a lot less important.

Okay, so it sounds like changing the message is enough for v15? The
other two things mentioned are nice-to-haves, but wouldn't need to
hold back this minimal change, it seems:

On Fri, Feb 4, 2022 at 4:50 AM Andres Freund  wrote:

> I wonder if we shouldn't add some exceptions to the xid allocation
> prevention. It makes sense that we don't allow random DML. But it's e.g. often
> more realistic to drop / truncate a few tables with unimportant content,
> rather than spend the time vacuuming those.  We could e.g. allow xid
> consumption within VACUUM, TRUNCATE, DROP TABLE / INDEX when run at the top
> level for longer than we allow it for anything else.

It seems like this would require having access to "nodetag(parsetree)"
of the statement available in GetNewTransactionId. I don't immediately
see an easy way to do that...is a global var within the realm of
acceptability?

On Fri, Feb 4, 2022 at 8:35 AM Andres Freund  wrote:

> we'd actually tell the user a bit more what about what is causing the
> problem.
>
> We can compute the:
> 1) oldest slot by xmin, with name
> 2) oldest walsender by xmin, with pid
> 3) oldest prepared transaction id by xid / xmin, with name
> 4) oldest in-progress transaction id by xid / xmin, with name
> 5) oldest database datfrozenxid, with database name
[...]
> Also, adding an SRF providing the above in a useful format would be great for
> monitoring and for "remote debugging" of problems.

I concur it sounds very useful, and not terribly hard, but probably a
v16 project.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

[Michael Paquier]

On Sat, Feb 12, 2022 at 08:50:41PM -0800, Andres Freund wrote:
> On 2022-01-18 11:20:16 +0900, Michael Paquier wrote:
>> +# required for 002_pg_upgrade.pl
>> +REGRESS_SHLIB=$(abs_top_builddir)/src/test/regress/regress$(DLSUFFIX)
>> +export REGRESS_SHLIB
> 
> It seems weird to propagate this into multiple places. Why don't we define
> that centrally?
> 
> Although it's weird for this to use REGRESS_SHLIB, given it's just doing
> dirname() on it. 027_stream_regress.pl has the "defense" of not wanting to
> duplicate the variable with 017_shm.pl...
> 
> Not that I understand why 017_shm.pl and all the regression test source
> fileseven need $(DLSUFFIX) - expand_dynamic_library_name() should take care of
> it?

I agree that we should be able to get rid of that in the long-term,
but this also feels like a separate issue to me and the patch is
already doing a lot.  I am wondering about the interactions of
installcheck with abs_top_builddir, though.  Should it be addressed
first?  It does not feel like a mandatory requirement for this
thread, anyway.

> It's not like make / msvc put the data in different places:
> src/test/recovery/Makefile:REGRESS_OUTPUTDIR=$(abs_top_builddir)/src/test/recovery/tmp_check
> src/tools/msvc/vcregress.pl: $ENV{REGRESS_OUTPUTDIR} = 
> "$topdir/src/test/recovery/tmp_check";

Yeah, removed.

>> +# From now on, the test of pg_upgrade consists in setting up an instance.
> 
> What does "from now on" mean?

In this context, the next steps of the test.  Removed.

>> +# Default is the location of this source code for both nodes used with
>> +# the upgrade.
> 
> Can't quite parse.

Reworded, to something hopefully better.

>> +# Initialize a new node for the upgrade.  This is done early so as it is
>> +# possible to know with which node's PATH the initial dump needs to be
>> +# taken.
>> +my $newnode = PostgreSQL::Test::Cluster->new('new_node');
>> +$newnode->init(extra => [ '--locale=C', '--encoding=LATIN1' ]);
>> +my $newbindir = $newnode->config_data('--bindir');
>> +my $oldbindir = $oldnode->config_data('--bindir');
> 
> Why C/LATIN?

Well, these are bits from the patch that I have played with
extensively, and it took me some time to remember why this was needed.
The reason why I introduced this option is that the patch created the
database "regression" using a createdb command that would feed from
template1 as pg_regress used --use-existing.  And this combination
required to enforce --locale=C to avoid two regression diffs in
int8.sql and numeric.sql.  It is possible to simplify things by
removing --use-existing and the database creation, so as pg_regress
handles the creation of the database "regression" with template0 to
avoid any problems related to locales.

Now, if you do *not* do that, I have noticed that we run into problems
when testing the TAP script with older versions, where pg_regress
would may not create the "regression" database, hence requiring an
extra createdb (perhaps that's better with --locale=C and
--template=template0) with --use-existing present for the pg_regress
command, command coming from the old branch.

Hmm.  At the end of the day, I am wondering whether we should not give
up entirely on the concept of running the regression tests on older
branches in the TAP script of a newer branch.  pg_regress needs to
come from the old source tree, meaning that we would most likely need
to maintain a set of compatibility tweaks that would most probably
rot over the time, and the buildfarm only cares about the possibility
to set up old instances by loading dumps rather than running
pg_regress.  This would also make the switch to TAP much easier (no
need for the extra createdb or --locale AFAIK).  So attempting to
maintain all that is going to be a PITA in the long term, and there is
nothing running that automatically anyway.

There is also the extra requirement to adjust dump files, but that's
independent of setting up the old instance to upgrade, and I don't
really plan to tackle that as of this thread (note that the buildfarm
client has extra tweaks regarding that).

Any thoughts about that?

> Right now pg_upgrade test.sh uses --wal-segsize 1, and that has helped
> identify several bugs. So I'd rather not give it up, even if it's a bit weird.

--allow-group-access was missing as well.

>> +my @regress_command = [
>> +$ENV{PG_REGRESS},
>> +'--schedule', "$oldsrc/src/test/regress/parallel_schedule",
>> +'--bindir',   $oldnode->config_data('--bindir'),
>> +'--dlpath',   $dlpath,
>> +'--port', $oldnode->port,
>> +'--outputdir',$outputdir,
>> +'--inputdir', $inputdir,
>> +'--use-existing'
>> +];
> 
> I think this should use --host (c.f. 7340aceed72). Or is it intending to use
> the host via env? If so, why is the port specified?

Hm.  It looks like you are right here, so added.

>> +@regress_command = (@regress_command, @extra_opts);
>> +
>> 

Re: Better error message for unsupported replication cases

[Tom Lane]

Amit Kapila  writes:
> On Tue, Feb 15, 2022 at 3:42 AM Tom Lane  wrote:
>> ...  Maybe the error message
>> could be bikeshedded ... is "non-table relation" terminology
>> that we use in user-facing messages?

> The other option could be "logical replication source relation
> \"%s.%s\" is not a table". We use a similar message in
> CheckSubscriptionRelkind.

Works for me, I'll do it like that if there are no objections.

regards, tom lane

Re: Time to drop plpython2?

[Tom Lane]

Andres Freund  writes:
> On February 14, 2022 12:48:12 PM PST, Tom Lane  wrote:
>> We could I guess, but does it really buy anything?  I'm sure that
>> some of the buildfarm still hasn't updated their Python installation,
>> but it'll be about the same failure we'd get from the final patch.

> I guess what I was actually wondering about - but didn't write - was whether 
> it's worth rejecting python 2 with just  configure / msvc perl changes 
> initially.

Ah, I see.  +1, that would let us start nagging the laggard buildfarm
owners right away.  But I do think we want the rest of the cleanup
done pretty soon --- especially simplification of the plpython
regression test arrangements.

regards, tom lane

Re: Better error message for unsupported replication cases

[Amit Kapila]

On Tue, Feb 15, 2022 at 3:42 AM Tom Lane  wrote:
>
> In [1] there's a complaint that if you try to logically replicate
> a partitioned table from v13-or-later to v12-or-earlier, you get
> "table XXX not found on publisher", which is pretty confusing
> because the publisher certainly does have such a table.  That
> happens because fetch_remote_table_info is too aggressive about
> filtering by relkind and doesn't see the relation at all.
> c314c147c improved that, but it wasn't back-patched.  I propose
> putting the attached into v10-v12.  Maybe the error message
> could be bikeshedded ... is "non-table relation" terminology
> that we use in user-facing messages?
>

The other option could be "logical replication source relation
\"%s.%s\" is not a table". We use a similar message in
CheckSubscriptionRelkind.

-- 
With Regards,
Amit Kapila.

Re: [BUG]Update Toast data failure in logical replication

[Andres Freund]

Hi,

On 2022-02-14 14:54:41 +0530, Amit Kapila wrote:
> On Fri, Feb 11, 2022 at 4:27 PM Amit Kapila  wrote:
> >
> > On Fri, Feb 11, 2022 at 12:00 PM tanghy.f...@fujitsu.com
> >  wrote:
> > >
> > > Attached the patches which fixed the above two comments and the first 
> > > comment in
> > > my previous mail [1], the rest is the same as before.
> > > I ran the tests on all branches, they all passed as expected.
> > >
> >
> > Thanks, these look good to me. I'll push these early next week
> > (Monday) unless there are any more suggestions or comments.
> >
> 
> Pushed!

Thanks for all the work on this!

Re: Replication slot drop message is sent after pgstats shutdown.

[Kyotaro Horiguchi]

At Mon, 14 Feb 2022 17:20:16 -0800, Andres Freund  wrote in 
> Hi,
> 
> On 2021-12-22 22:34:45 +0900, Masahiko Sawada wrote:
> > I've attached an updated patch. Please review it.
> 
> Sorry for dropping the ball on this again :(. I've pushed the fix with some
> very minor polishing.

Thanks!

> > > The attached detects that bug, but I'm not sure it's worth expending
> > > test time, or this might be in the server test suit.
> >
> > Thanks. It's convenient to test this issue but I'm also not sure it's
> > worth adding to the test suit.
> 
> I think it's definitely worth adding a test, but I don't particularly like the
> specific test implementation. Primarily because I think it's better to test
> this in a cluster that stays running, so that we can verify that the slot drop
> worked. It also doesn't seem necessary to create a separate cluster.

One of the points I was not satisfied the TAP test is the second point
above.  FWIW I agree to the proposed test on the direction.

> I wrote the attached isolation test. I ended up not committing it yet - I'm
> worried that there could be some OS dependent output difference, due to some
> libpq error handling issues. See [1], which Tom pointed out is caused by the
> issue discussed in [2].

Mmm.. This is..
slot_creation_error.out
> step s2_init: <... completed>
> FATAL:  terminating connection due to administrator command
> FATAL:  terminating connection due to administrator command

> Greetings,
> 
> Andres Freund
> 
> [1] https://postgr.es/m/20220215004143.dlzsn72oqsmqa7uw%40alap3.anarazel.de
> [2] https://postgr.es/m/20220215004143.dlzsn72oqsmqa7uw%40alap3.anarazel.de

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Time to drop plpython2?

[Andres Freund]

Hi, 

On February 14, 2022 12:48:12 PM PST, Tom Lane  wrote:
>Andres Freund  writes:
>> On 2022-02-14 14:18:58 -0500, Tom Lane wrote:
>>> Well, it's mid-February.  Do we have a python2-removal patch
>>> that's ready to go?
>
>> I can refresh mine. Iit might be good to first reapply
>> f201da39edc - "Make configure prefer python3 to plain python."
>> for a few days?
>
>We could I guess, but does it really buy anything?  I'm sure that
>some of the buildfarm still hasn't updated their Python installation,
>but it'll be about the same failure we'd get from the final patch.

I guess what I was actually wondering about - but didn't write - was whether 
it's worth rejecting python 2 with just  configure / msvc perl changes 
initially.

The proper removal of python 2 support includes a bunch of buildsystem and code 
changes. Seems like it could be useful to have a snapshot of the buildfarm 
state after rejecting python 2, separate from the more verbose changes.

Andres


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

Re: Make mesage at end-of-recovery less scary.

[Kyotaro Horiguchi]

At Mon, 14 Feb 2022 20:14:11 +0530, Ashutosh Sharma  
wrote in 
> No, I haven't tried to compare archive recovery to PITR or vice versa,
> instead I was trying to compare crash recovery with PITR. The message
> you're emitting says just before entering into the archive recovery is
> - "reached end-of-WAL on ... in pg_wal *during crash recovery*,
> entering archive recovery".  This message is static and can be emitted
> not only during crash recovery, but also during PITR. I think we can

No. It is emitted *only* after crash recovery before starting archive
recovery.  Another message this patch adds can be emitted after PITR
or archive recovery.

> not only during crash recovery, but also during PITR. I think we can
> remove the "during crash recovery" part from this message, so "reached
> the end of WAL at %X/%X on timeline %u in %s, entering archive

What makes you think it can be emitted after other than crash recovery?
(Please look at the code comment just above.)

> recovery". Also I don't think we need format specifier %s here, it can
> be hard-coded with pg_wal as in this case we can only enter archive
> recovery after reading wal from pg_wal, so current WAL source has to
> be pg_wal, isn't it?

You're right that it can't be other than pg_wal.  It was changed just
in accordance woth another message this patch adds and it would be a
matter of taste.  I replaced to "pg_wal" in this version.

> Thanks for the changes. Please note that I am not able to apply the
> latest patch on HEAD. Could you please rebase it on HEAD and share the
> new version. Thank you.

A change on TAP script hit this.  The v13 attached is:

- Rebased.

- Replaced "%s" in the debug transition message from crash recovery to
  archive recovery.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 311e862e87dbdeb6348c6fc17063308342359c02 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Fri, 28 Feb 2020 15:52:58 +0900
Subject: [PATCH v13] Make End-Of-Recovery error less scary

When recovery in any type ends, we see a bit scary error message like
"invalid record length" that suggests something serious is
happening. Actually if recovery meets a record with length = 0, that
usually means it finished applying all available WAL records.

Make this message less scary as "reached end of WAL". Instead, raise
the error level for other kind of WAL failure to WARNING.
---
 src/backend/access/transam/xlog.c |  91 ++-
 src/backend/access/transam/xlogreader.c   |  78 
 src/backend/replication/walreceiver.c |   7 +-
 src/bin/pg_waldump/pg_waldump.c   |  13 ++-
 src/include/access/xlogreader.h   |   1 +
 src/test/recovery/t/011_crash_recovery.pl | 106 ++
 6 files changed, 266 insertions(+), 30 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 958220c495..bb7026ac77 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4480,6 +4480,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 	for (;;)
 	{
 		char	   *errormsg;
+		XLogRecPtr	ErrRecPtr = InvalidXLogRecPtr;
 
 		record = XLogReadRecord(xlogreader, );
 		if (record == NULL)
@@ -4495,6 +4496,18 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			{
 abortedRecPtr = xlogreader->abortedRecPtr;
 missingContrecPtr = xlogreader->missingContrecPtr;
+ErrRecPtr = abortedRecPtr;
+			}
+			else
+			{
+/*
+ * EndRecPtr is the LSN we tried to read but failed. In the
+ * case of decoding error, it is at the end of the failed
+ * record but we don't have a means for now to know EndRecPtr
+ * is pointing to which of the beginning or ending of the
+ * failed record.
+ */
+ErrRecPtr = xlogreader->EndRecPtr;
 			}
 
 			if (readFile >= 0)
@@ -4504,13 +4517,16 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			}
 
 			/*
-			 * We only end up here without a message when XLogPageRead()
-			 * failed - in that case we already logged something. In
-			 * StandbyMode that only happens if we have been triggered, so we
-			 * shouldn't loop anymore in that case.
+			 * We only end up here without a message when XLogPageRead() failed
+			 * in that case we already logged something, or just met end-of-WAL
+			 * conditions. In StandbyMode that only happens if we have been
+			 * triggered, so we shouldn't loop anymore in that case. When
+			 * EndOfWAL is true, we don't emit that error if any immediately
+			 * and instead will show it as a part of a decent end-of-wal
+			 * message later.
 			 */
-			if (errormsg)
-ereport(emode_for_corrupt_record(emode, xlogreader->EndRecPtr),
+			if (!xlogreader->EndOfWAL && errormsg)
+ereport(emode_for_corrupt_record(emode, ErrRecPtr),
 		(errmsg_internal("%s", errormsg) /* already translated */ ));
 		}
 
@@ -4541,11 +4557,14 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
 			/* Great, got a record */
 			return 

Re: automatically generating node support functions

[Andres Freund]

Hi,

On 2022-02-14 20:47:33 -0500, Tom Lane wrote:
> I think that most of the intellectual content in this patch is getting
> the data source nailed down, ie putting the annotations into the *nodes.h
> files and building the code to parse that.  I don't have a problem
> with throwing away and rewriting the back-end part of the patch later.

Imo that cuts the other way - without going for a metadata based approach we
don't know if we made the annotations rich enough...


> And, TBH, I am not really convinced that a pure metadata approach is going
> to work out, or that it will have sufficient benefit over just automating
> the way we do it now.  I notice that Peter's patch leaves a few
> too-much-of-a-special-case functions unconverted, which is no real
> problem for his approach; but it seems like you won't get to take such
> shortcuts in a metadata-reading implementation.

IMO my prototype of that approach pretty conclusively shows that it's feasible
and worthwhile.


> The bottom line here is that I believe that Peter's patch could get us out
> of the business of hand-maintaining the backend/nodes/*.c files in the v15
> timeframe, which would be a very nice thing.  I don't see how your patch
> will be ready on anywhere near the same schedule.  When it is ready, we can
> switch, but in the meantime I'd like the maintenance benefit.

I'm not going to try to prevent the patch from going in. But I don't think
it's a great idea to this without even trying to ensure the annotations are
rich enough...

Greetings,

Andres Freund

Re: Postgres 14.2 Windows can't rename temporary statistics file

[Thomas Munro]

On Tue, Feb 15, 2022 at 2:09 PM Ranier Vilela  wrote:
> Em seg., 14 de fev. de 2022 às 21:58, Justin Pryzby  
> escreveu:
>> On Mon, Feb 14, 2022 at 09:19:54PM -0300, Ranier Vilela wrote:
>> > I've reported this issue, but without success in fixing it.
>>
>> It'd be helpful to provide a link to the prior discussions, and summarize it.
>>
>> https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items
>> https://www.postgresql.org/message-id/CAEudQAoR5e7=umz0otzucvb25ztc8qqbe+2dt1jrsa3u+xu...@mail.gmail.com
>>
>> See also e2f0f8ed251d02c1eda79e1ca3cb3db2681e7a86
>
> I can't be sure if this is a case of DELETE_PENDING,
> but FlieMoveEx apparently solves it.

Can you explain why it solves it?

Have you seen this?

https://commitfest.postgresql.org/37/3347/

Do you know if FileMoveEx might be magically doing POSIX-style
operations when NTFS is used, on some recent version of Windows, as
was described at [1][2] with respect to DeleteFileEx?  CF #3347 is
about explicitly asking for those semantics, but [1] says that the
need to do that might be going away.

One of the reasons I have been putting off committing #3347 (other
than my desire to remove the pre-Win10 support) is (1) the hypothesis
that the problem might become simpler to solve as of some Windows 10
version as you might be seeing with your FileMoveEx test and (2) I'm
not sure how to handle the fact that once all the build farm animals
and CI systems are using POSIX semantics on NTFS, we'll have no
testing of the non-POSIX/traditional Windows semantics.  I mean stuff
like e2f0f8ed.  I suppose we really should have a non-NTFS build farm
animal, and I think we should have something like pg_test_dirmod.exe
that runs through a bunch of  standalone tests.

[1] 
https://stackoverflow.com/questions/60424732/did-the-behaviour-of-deleted-files-open-with-fileshare-delete-change-on-windows
[2] 
https://www.postgresql.org/message-id/20210905214437.y25j42yigwnbd...@alap3.anarazel.de

Re: Mark all GUC variable as PGDLLIMPORT

[Andres Freund]

Hi,

On 2022-02-14 12:45:08 -0500, Robert Haas wrote:
> On Mon, Feb 14, 2022 at 12:34 PM Tom Lane  wrote:
> > * If we institute a policy that all GUCs should be PGDLLIMPORT'd,
> > how will we enforce that new patches follow that?  I don't want to be
> > endlessly going back and adding forgotten PGDLLIMPORT markers.
> 
> It seems to me that it would be no different from bumping catversion
> or updating nodefuncs.c: yes, it will get forgotten sometimes, but
> hopefully people will mostly get used to it just like they do with
> dozens of other PG-specific coding rules. I don't see that it's likely
> to generate more than a handful of commits per year, so it doesn't
> really seem worth worrying about to me, but YMMV. Maybe it's possible
> to write a perl script to verify it, although it seems like it might
> be complicated to code that up.

I think it'd be better if we had scripts ensuring this stays sane. Several of
your examples do have that. Whereas I don't see what would cause us to missing
PGDLLIMPORTs for GUCs, given it's a windows only issue and that you need an
extension using the GUC with omitted PGDLLIMPORT.


> An alternative rule which would dodge that particular issue would be
> to just slap PGDLLIMPORT on every global variable in every header
> file. That would arguably be a simpler rule, though it means even more
> PGDLLIMPORT declarations floating around.

That would have the advantage of being comparatively easy to check in an
automated way. Might even be cheap enough to just make it part of the build.

But it seems like it'd be a fair amount of work and cause a lot of patch
rebasing pain? If we end up going that way, we should schedule this to happen
just after the feature freeze, I think.

If we consider doing this for all extern variables, we should think about
doing this for headers *and* functions. That'd allow us to get rid of the
fairly complicated logic to generate the .def file for the postgres binary on
windows (src/tools/gendef.pl). And maybe also the related thing on AIX
(src/backend/port/aix/mkldexport.sh)

I'm kind of partial to the "add explicit visibility information to everything"
approach because the windows export file generation is a decent chunk of the
meson patchset. And what's missing to make it work on AIX...

Greetings,

Andres Freund

Re: automatically generating node support functions

[Tom Lane]

Andres Freund  writes:
> On 2022-02-14 18:32:21 -0500, Tom Lane wrote:
>> I think that finishing out and committing this patch is a fine step
>> on the way to that.

> I think most of gen_node_support.pl would change - a lot of that is generating
> the node functions, which would not be needed anymore. And most of the
> remainder would change as well.

Well, yeah, we'd be throwing away some of that Perl code.  So what?
I think that most of the intellectual content in this patch is getting
the data source nailed down, ie putting the annotations into the *nodes.h
files and building the code to parse that.  I don't have a problem
with throwing away and rewriting the back-end part of the patch later.

And, TBH, I am not really convinced that a pure metadata approach is going
to work out, or that it will have sufficient benefit over just automating
the way we do it now.  I notice that Peter's patch leaves a few
too-much-of-a-special-case functions unconverted, which is no real
problem for his approach; but it seems like you won't get to take such
shortcuts in a metadata-reading implementation.

The bottom line here is that I believe that Peter's patch could get us out
of the business of hand-maintaining the backend/nodes/*.c files in the
v15 timeframe, which would be a very nice thing.  I don't see how your
patch will be ready on anywhere near the same schedule.  When it is ready,
we can switch, but in the meantime I'd like the maintenance benefit.

regards, tom lane

Re: Fix overflow in justify_interval related functions

[Joseph Koshakow]

On Mon, Feb 14, 2022 at 7:59 PM Nathan Bossart  wrote:
> I think it's possible to avoid overflow in justify_interval() in some cases
> by pre-justifying the days.  I've attached a patch to demonstrate.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Good catch, I didn't think about that. Though if you are pre-justifying
the days, then I don't think it's possible for the second addition to
days to overflow. The maximum amount the days field could be after
the first justification is 29. I think we can simplify it further to the
attached patch.

- Joe
From 8ff8302b90c790d967f770286dfa54432f63662c Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.

Signed-off-by: Joseph Koshakow 
---
 src/backend/utils/adt/timestamp.c  | 29 ++---
 src/test/regress/expected/interval.out | 36 ++
 src/test/regress/sql/interval.sql  | 12 +
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..ce11fcbeef 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2717,12 +2717,27 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->day = span->day;
 	result->time = span->time;
 
+	/* pre-justify days if it might prevent overflow */
+	if ((result->day > 0 && result->time > 0) ||
+		(result->day < 0 && result->time < 0))
+	{
+		wholemonth = result->day / DAYS_PER_MONTH;
+		result->day -= wholemonth * DAYS_PER_MONTH;
+		if (pg_add_s32_overflow(result->month, wholemonth, >month))
+			ereport(ERROR,
+	(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+	 errmsg("interval out of range")));
+	}
+
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	result->day += wholeday;
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, >month))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2787,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, >day))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2826,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, >month))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..146f7c55d0 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,38 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+   justify_interval
+---
+ @ 5965232 years 4 mons 8 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+ justify_interval  
+---
+ @ 5965232 years 4 mons 9 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months -30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+ justify_interval 
+--
+ @ 178956970 years 7 mons 29 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+   justify_interval   

Re: automatically generating node support functions

[Andres Freund]

Hi,

On 2022-02-14 18:32:21 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I do however not think it's a good idea to commit something generating
> > something like the existing node functions vs going for a metadata based
> > approach at dealing with node functions. That aspect of my patchset is
> > independent of the libclang vs script debate.
> 
> I think that finishing out and committing this patch is a fine step
> on the way to that.

I think most of gen_node_support.pl would change - a lot of that is generating
the node functions, which would not be needed anymore. And most of the
remainder would change as well.


> Either that, or you should go ahead and merge your backend work onto what
> Peter's done ...

I did offer to do part of that a while ago:

https://www.postgresql.org/message-id/20210715012454.bvwg63farhmfwb47%40alap3.anarazel.de

On 2021-07-14 18:24:54 -0700, Andres Freund wrote:
> If Peter could generate something roughly like the metadata I emitted, I'd
> rebase my node functions ontop of that.

Greetings,

Andres Freund

Re: Assertion failure in pgstat_assert_is_up during walsender exit

[Andres Freund]

Hi,

On 2022-02-14 14:36:12 -0800, Andres Freund wrote:
> On 2022-02-14 15:45:40 -0500, Tom Lane wrote:
> > I tried to replicate the problem described in [1] about logical
> > replication from a v14 source DB to a v11 target.  It fails as
> > described there; I've not yet tracked down why, but it looks like
> > the v11 apply worker fails and closes the connection after sending
> > CREATE_REPLICATION_SLOT.  The v14 walsender doesn't have a problem
> > with that.  However, if I do the same thing using HEAD as the source,
> > I see an assertion failure on the way out of the walsender.  The
> > postmaster log shows
> 
> Gah, sorry, forgot about this one. I'll fix it / apply the fix.

Pushed Sawada-san's fix now. Left the test ([1]) for later, until I can figure
out if the test would be stable on all platforms WRT the duplicated FATAL.

Greetings,

Andres Freund

[1] https://postgr.es/m/20220215012016.lbch4tvpxnm4y7ec%40alap3.anarazel.de

Re: Replication slot drop message is sent after pgstats shutdown.

[Andres Freund]

Hi,

On 2021-12-22 22:34:45 +0900, Masahiko Sawada wrote:
> I've attached an updated patch. Please review it.

Sorry for dropping the ball on this again :(. I've pushed the fix with some
very minor polishing.


> > The attached detects that bug, but I'm not sure it's worth expending
> > test time, or this might be in the server test suit.
>
> Thanks. It's convenient to test this issue but I'm also not sure it's
> worth adding to the test suit.

I think it's definitely worth adding a test, but I don't particularly like the
specific test implementation. Primarily because I think it's better to test
this in a cluster that stays running, so that we can verify that the slot drop
worked. It also doesn't seem necessary to create a separate cluster.

I wrote the attached isolation test. I ended up not committing it yet - I'm
worried that there could be some OS dependent output difference, due to some
libpq error handling issues. See [1], which Tom pointed out is caused by the
issue discussed in [2].

Greetings,

Andres Freund

[1] https://postgr.es/m/20220215004143.dlzsn72oqsmqa7uw%40alap3.anarazel.de
[2] https://postgr.es/m/20220215004143.dlzsn72oqsmqa7uw%40alap3.anarazel.de
>From 343106442eafde4fcc55cc75e78501010d50a883 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Mon, 14 Feb 2022 17:17:37 -0800
Subject: [PATCH v1] Add isolation test for errors during logical slot
 creation.

Discussion: https://postgr.es/m/cad21aodaeepabzeyyjspzjumsparicvsbobal7spaofnkz+...@mail.gmail.com
---
 contrib/test_decoding/Makefile|   2 +-
 .../expected/slot_creation_error.out  | 113 ++
 .../specs/slot_creation_error.spec|  41 +++
 3 files changed, 155 insertions(+), 1 deletion(-)
 create mode 100644 contrib/test_decoding/expected/slot_creation_error.out
 create mode 100644 contrib/test_decoding/specs/slot_creation_error.spec

diff --git a/contrib/test_decoding/Makefile b/contrib/test_decoding/Makefile
index 56ddc3abaeb..36929dd97d3 100644
--- a/contrib/test_decoding/Makefile
+++ b/contrib/test_decoding/Makefile
@@ -9,7 +9,7 @@ REGRESS = ddl xact rewrite toast permissions decoding_in_xact \
 	sequence
 ISOLATION = mxact delayed_startup ondisk_startup concurrent_ddl_dml \
 	oldest_xmin snapshot_transfer subxact_without_top concurrent_stream \
-	twophase_snapshot
+	twophase_snapshot slot_creation_error
 
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
 ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf
diff --git a/contrib/test_decoding/expected/slot_creation_error.out b/contrib/test_decoding/expected/slot_creation_error.out
new file mode 100644
index 000..fc1c98b5248
--- /dev/null
+++ b/contrib/test_decoding/expected/slot_creation_error.out
@@ -0,0 +1,113 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_b s1_xid s2_init s1_view_slot s1_cancel_s2 s1_view_slot s1_c
+step s1_b: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s1_xid: SELECT 'xid' FROM txid_current();
+?column?
+
+xid 
+(1 row)
+
+step s2_init: 
+SELECT 'init' FROM pg_create_logical_replication_slot('slot_creation_error', 'test_decoding');
+ 
+step s1_view_slot: 
+SELECT slot_name, slot_type, active FROM pg_replication_slots WHERE slot_name = 'slot_creation_error'
+
+slot_name  |slot_type|active
+---+-+--
+slot_creation_error|logical  |t 
+(1 row)
+
+step s1_cancel_s2: 
+SELECT pg_cancel_backend(pid)
+FROM pg_stat_activity
+WHERE application_name = 'isolation/slot_creation_error/s2';
+
+pg_cancel_backend
+-
+t
+(1 row)
+
+step s2_init: <... completed>
+ERROR:  canceling statement due to user request
+step s1_view_slot: 
+SELECT slot_name, slot_type, active FROM pg_replication_slots WHERE slot_name = 'slot_creation_error'
+
+slot_name|slot_type|active
+-+-+--
+(0 rows)
+
+step s1_c: COMMIT;
+
+starting permutation: s1_b s1_xid s2_init s1_c s1_view_slot s1_drop_slot
+step s1_b: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s1_xid: SELECT 'xid' FROM txid_current();
+?column?
+
+xid 
+(1 row)
+
+step s2_init: 
+SELECT 'init' FROM pg_create_logical_replication_slot('slot_creation_error', 'test_decoding');
+ 
+step s1_c: COMMIT;
+step s2_init: <... completed>
+?column?
+
+init
+(1 row)
+
+step s1_view_slot: 
+SELECT slot_name, slot_type, active FROM pg_replication_slots WHERE slot_name = 'slot_creation_error'
+
+slot_name  |slot_type|active
+---+-+--
+slot_creation_error|logical  |f 
+(1 row)
+
+step s1_drop_slot: 
+SELECT pg_drop_replication_slot('slot_creation_error');
+
+pg_drop_replication_slot
+
+
+(1 row)
+
+
+starting permutation: s1_b s1_xid s2_init s1_terminate_s2 s1_c s1_view_slot
+step s1_b: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step s1_xid: SELECT 'xid' FROM txid_current();
+?column?

Re: Postgres 14.2 Windows can't rename temporary statistics file

[Ranier Vilela]

Em seg., 14 de fev. de 2022 às 21:58, Justin Pryzby 
escreveu:

> On Mon, Feb 14, 2022 at 09:19:54PM -0300, Ranier Vilela wrote:
> > I've reported this issue, but without success in fixing it.
>
> It'd be helpful to provide a link to the prior discussions, and summarize
> it.
>
> https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items
>
> https://www.postgresql.org/message-id/CAEudQAoR5e7=umz0otzucvb25ztc8qqbe+2dt1jrsa3u+xu...@mail.gmail.com
>
> See also e2f0f8ed251d02c1eda79e1ca3cb3db2681e7a86
>
I can't be sure if this is a case of DELETE_PENDING,
but FlieMoveEx apparently solves it.

regards,
Ranier Vilela

Re: Fix overflow in justify_interval related functions

[Nathan Bossart]

On Mon, Feb 14, 2022 at 04:57:07PM -0500, Joseph Koshakow wrote:
> On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart  
> wrote:
>> Makes sense.  So we could likely avoid it for justify_interval, but the
>> others are at the mercy of the interval implementation.
> 
> I'm not entirely sure what you mean by "it", but for both
> justify_interval and justify_days this commit throws an error if we
> try to set the months field above 2^31.
> 
>> +SELECT justify_days(interval '2147483647 months 30 days');
>> +ERROR:  interval out of range
>> +SELECT justify_interval(interval '2147483647 months 30 days');
>> +ERROR:  interval out of range
> 
> That's what these are testing.

I think it's possible to avoid overflow in justify_interval() in some cases
by pre-justifying the days.  I've attached a patch to demonstrate.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From d566fcc8cf0179bb915db828b528df55484b63dc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow 
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH v2 1/1] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.
---
 src/backend/utils/adt/timestamp.c  | 32 ---
 src/test/regress/expected/interval.out | 36 ++
 src/test/regress/sql/interval.sql  | 12 +
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..8cda47e9a4 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2717,12 +2717,30 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->day = span->day;
 	result->time = span->time;
 
+	/* pre-justify days if it might prevent overflow */
+	if ((result->day > 0 && result->time > 0) ||
+		(result->day < 0 && result->time < 0))
+	{
+		wholemonth = result->day / DAYS_PER_MONTH;
+		result->day -= wholemonth * DAYS_PER_MONTH;
+		if (pg_add_s32_overflow(result->month, wholemonth, >month))
+			ereport(ERROR,
+	(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+	 errmsg("interval out of range")));
+	}
+
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, >day))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, >month))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2790,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, >day))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2829,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, >month))
+		ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..146f7c55d0 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,38 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+   justify_interval
+---
+ @ 5965232 years 4 mons 8 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+ justify_interval  
+---
+ @ 5965232 years 4 mons 9 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of 

Re: support for CREATE MODULE

[Bruce Momjian]

On Mon, Feb 14, 2022 at 07:42:21PM -0500, Bruce Momjian wrote:
> On Mon, Feb 14, 2022 at 03:23:07PM -0800, Swaha Miller wrote:
> > A prominent use case for grouping functions into modules would
> > be access control on the group as a whole, with one command
> > for an entire module instead of many individual functions. One reason
> > for such a grouping is to set ACLs. Users migrating their database from
> > commercial databases to PostgreSQL wanting to set ACLs on
> > thousands or hundreds of thousands of functions would benefit from
> > a grouping concept like modules.
> > 
> > If users use schemas to group functions, then they have to break up
> > functions that may have all been in one namespace into a bunch of new
> > schemas. So we'd like to have a different grouping mechanism that can
> > group functions within a schema. So we're looking at a new construct like
> > modules that can serve that purpose without introducing sub-schemas.
> 
> I was working on a talk about microservices today and decided to create
> two schemas --- a public one that has USAGE permission for other services
> with views and SECURITY DEFINER functions that referenced a private
> schema that can only be accessed by the owning service.  Is that a usage
> pattern that requires modules since it already works with schemas and
> just uses schema permissions to designate public/private schema
> interfaces.

Attached is an example.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.



public-schema.sql
Description: application/sql

Re: Postgres 14.2 Windows can't rename temporary statistics file

[Justin Pryzby]

On Mon, Feb 14, 2022 at 09:19:54PM -0300, Ranier Vilela wrote:
> I've reported this issue, but without success in fixing it.

It'd be helpful to provide a link to the prior discussions, and summarize it.

https://wiki.postgresql.org/wiki/PostgreSQL_14_Open_Items
https://www.postgresql.org/message-id/CAEudQAoR5e7=umz0otzucvb25ztc8qqbe+2dt1jrsa3u+xu...@mail.gmail.com

See also e2f0f8ed251d02c1eda79e1ca3cb3db2681e7a86

-- 
Justin

Re: support for CREATE MODULE

[Bruce Momjian]

On Mon, Feb 14, 2022 at 03:23:07PM -0800, Swaha Miller wrote:
> A prominent use case for grouping functions into modules would
> be access control on the group as a whole, with one command
> for an entire module instead of many individual functions. One reason
> for such a grouping is to set ACLs. Users migrating their database from
> commercial databases to PostgreSQL wanting to set ACLs on
> thousands or hundreds of thousands of functions would benefit from
> a grouping concept like modules.
> 
> If users use schemas to group functions, then they have to break up
> functions that may have all been in one namespace into a bunch of new
> schemas. So we'd like to have a different grouping mechanism that can
> group functions within a schema. So we're looking at a new construct like
> modules that can serve that purpose without introducing sub-schemas.

I was working on a talk about microservices today and decided to create
two schemas --- a public one that has USAGE permission for other services
with views and SECURITY DEFINER functions that referenced a private
schema that can only be accessed by the owning service.  Is that a usage
pattern that requires modules since it already works with schemas and
just uses schema permissions to designate public/private schema
interfaces.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Postgres 14.2 Windows can't rename temporary statistics file

[Ranier Vilela]

Hi,

I've reported this issue, but without success in fixing it.
Now I have installed 14.2 and used in development environment, and
the log still reports:
2022-02-13 18:33:20.502 -03 [7976] LOG:  could not rename temporary
statistics file "pg_stat_tmp/global.tmp" to "pg_stat_tmp/global.stat":
Permission denied
2022-02-13 19:33:22.648 -03 [7976] LOG:  could not rename temporary
statistics file "pg_stat_tmp/global.tmp" to "pg_stat_tmp/global.stat":
Permission denied

This have consequences in performance or stability of the server?

After applying the patch attached, the log does not report this issue
and dir command show this:

C:\postgres\data>dir *.stat /s
 O volume na unidade C é hd
 O Número de Série do Volume é F470-0E50

 Pasta de C:\postgres\data\pg_stat_tmp

14/02/2022  21:15 1.670 db_0.stat
14/02/2022  21:1512.215 db_25214.stat
14/02/2022  21:15 1.335 global.stat
   3 arquivo(s) 15.220 bytes


The point is we can not relies on the "rename" function at Windows Side.
The local installation is made with user with full rights and permissions,
but
Postgres Windows still can't rename correctly.

thoughts?

regards,
Ranier Vilela


force_rename_temp_windows.patch
Description: Binary data

RE: Support escape sequence for cluster_name in postgres_fdw.application_name

[r.takahash...@fujitsu.com]

Hi Fujii san,


Thank you for updating the patch.
I have no additional comments.

Regards,
Ryohei Takahashi

Re: automatically generating node support functions

[Tom Lane]

Andres Freund  writes:
> I do however not think it's a good idea to commit something generating
> something like the existing node functions vs going for a metadata based
> approach at dealing with node functions. That aspect of my patchset is
> independent of the libclang vs script debate.

I think that finishing out and committing this patch is a fine step
on the way to that.  Either that, or you should go ahead and merge
your backend work onto what Peter's done ... but that seems like
it'll be bigger and harder to review.

regards, tom lane

Re: automatically generating node support functions

[Andres Freund]

Hi,

On 2022-02-14 12:09:47 -0500, Tom Lane wrote:
> I'm in favor of moving forward with this.  I do not like the
> libclang-based approach that Andres was pushing, because of the
> jump in developer tooling requirements that it'd cause.

FWIW, while I don't love the way the header parsing stuff in the patch (vs
using libclang or such), I don't have a real problem with it.

I do however not think it's a good idea to commit something generating
something like the existing node functions vs going for a metadata based
approach at dealing with node functions. That aspect of my patchset is
independent of the libclang vs script debate.

Greetings,

Andres Freund

Re: support for CREATE MODULE

[Swaha Miller]

On Thu, Feb 10, 2022 at 1:06 PM Bruce Momjian  wrote:

> On Thu, Feb 10, 2022 at 08:53:15AM -0800, Swaha Miller wrote:
> > On Fri, Feb 4, 2022 at 3:51 PM Tom Lane  wrote:
> >
> > Hm. If the functional requirement is "group objects without needing
> > any out-in-the-filesystem infrastructure", then I could see defining
> > a module as being exactly like an extension except there's no such
> > infrastructure --- and hence no concept of versions, plus pg_dump
> > needs to act differently.  That's probably enough semantic difference
> > to justify using a separate word, even if we can share a lot of
> > code infrastructure.
> >
> > Then as a first cut for modules, could we add CREATE MODULE
> > syntax which adds an entry to pg_extension like CREATE EXTENSION
> > does? And also add a new column to pg_extension to distinguish
> > modules from extensions.
> >
> > The three-part path name resolution for functions would remain the
> > same, nothing would need to change there because of modules.
> >
> > Would that be an acceptable direction to go?
>
> Well, that would allow us to have CREATE EXTENSION syntax, but what
> would it do that CREATE SCHEMA does not?
>

A prominent use case for grouping functions into modules would
be access control on the group as a whole, with one command
for an entire module instead of many individual functions. One reason
for such a grouping is to set ACLs. Users migrating their database from
commercial databases to PostgreSQL wanting to set ACLs on
thousands or hundreds of thousands of functions would benefit from
a grouping concept like modules.

If users use schemas to group functions, then they have to break up
functions that may have all been in one namespace into a bunch of new
schemas. So we'd like to have a different grouping mechanism that can
group functions within a schema. So we're looking at a new construct like
modules that can serve that purpose without introducing sub-schemas.

Also a module would be a grouping of primarily functions. Schemas can
have many other object types. Setting ACLs on groups of functions in a
schema, which is what users could do with a module, would require
breaking up a schema into other schemas based on ACLs. But schemas
allow setting ACLs on other objects in it, not only functions. If we
create groupings based on functions, what happens to the other types
of objects, what schema do they get grouped into? If modules are
supposed to solve setting ACLs on groups of functions, does using
schemas conflate setting ACLs for those other types of objects with
setting ACLs for functions?

Modules could also, in the future, serve as a way of allowing for private
functions within the grouping. Doing this in schemas would require
those kinds of changes in the schema construct.

Swaha

Re: pgcrypto: Remove internal padding implementation

[Jacob Champion]

On Mon, 2022-02-14 at 10:42 +0100, Peter Eisentraut wrote:
> This is a rebase of the patch from [0].  It removes the internal padding 
> implementation in pgcrypto and lets OpenSSL do it.  The internal 
> implementation was once applicable to the non-OpenSSL code paths, but 
> those have since been removed.

These removed parts looked interesting to me:

> -   else if (bpos % bs)
> -   {
> -   /* ERROR? */
> -   pad = bs - (bpos % bs);
> -   for (i = 0; i < pad; i++)
> -   bbuf[bpos++] = 0;
> -   }

> -   /* unpad */
> -   if (bs > 1 && cx->padding)
> -   {
> -   pad = res[*rlen - 1];
> -   pad_ok = 0;
> -   if (pad > 0 && pad <= bs && pad <= *rlen)
> -   {
> -   pad_ok = 1;
> -   for (i = *rlen - pad; i < *rlen; i++)
> -   if (res[i] != pad)
> -   {
> -   pad_ok = 0;
> -   break;
> -   }
> -   }
> -
> -   if (pad_ok)
> -   *rlen -= pad;
> -   }

After this patch, bad padding is no longer ignored during decryption,
and encryption without padding now requires the input size to be a
multiple of the block size. To see the difference you can try the
following queries with and without the patch:

select encrypt_iv('foo', '0123456', 'abcd', 'aes/pad:none');
select 
encode(decrypt_iv('\xa21a9c15231465964e3396d32095e67eb52bab05f556a581621dee1b85385789',
 '0123456', 'abcd', 'aes'), 'escape');

Both changes seem correct to me. I can imagine some system out there
being somehow dependent on the prior decryption behavior to avoid a
padding oracle -- but if that's a concern, hopefully you're not using
unauthenticated encryption in the first place? It might be worth a note
in the documentation.

--Jacob

Re: fixing bookindex.html bloat

[Andres Freund]

Hi,

On 2022-02-14 23:06:20 +0100, Peter Eisentraut wrote:
> The attached patch cleans up the xhtml namespace declarations properly, I
> think.

Looks good to me.


> For the xlink business, I don't have a better idea than you, so your
> workaround proposal seems fine.

K. Will you apply your patch, and then I'll push mine ontop?

Greetings,

Andres Freund

Re: fairywren is generating bogus BASE_BACKUP commands

[Andres Freund]

Hi,

On 2022-02-14 17:32:11 -0500, Andrew Dunstan wrote:
> About the only thing missing in your recipe is this:

Re requiring out-of-tree builds: Thomas on IM noted that there's the
NoDefaultCurrentDirectoryInExePath environment variable. That should avoid the
problem leading to requiring out-of-tree builds. But I've not tested it.


> # force ucrt64 prove to use the ucrt64 perl rather than whatever is in
> the path
> sed -i 's,^#!perl,#!/ucrt64/bin/perl,' /ucrt64/bin/core_perl/prove
> 
> 
> Given that, you don't need to set PERL, and configure can find the perl
> to build against from the PATH.

That shouldn't even be needed from what I understand now. If correctly started
the msys shell shoul dhave the right perl in path?


Greetings,

Andres Freund

Re: Possible to go without page headers?

[Tom Lane]

Peter Geoghegan  writes:
> It isn't actually necessary for an index AM to use the standard
> slotted page format to get the benefits that you mention, of course --
> whether or not an index AM that uses standard page headers *also* uses
> slotted pages with standard line pointers is a separate question.

Right, you don't need to use a line pointer array if you don't want
to.  (IIRC, hash also opts out of that in some pages.)  I took the
question to be just about the page header proper.

regards, tom lane

Re: fairywren is generating bogus BASE_BACKUP commands

[Andres Freund]

On 2022-02-14 17:32:11 -0500, Andrew Dunstan wrote:
> Working on that. There appear to be some issues with third party
> libraries. I might need to rebuild libxml2 and zlib for example.

Any reason not to use the ones from msys2?

Re: Assertion failure in pgstat_assert_is_up during walsender exit

[Andres Freund]

Hi,

On 2022-02-14 15:45:40 -0500, Tom Lane wrote:
> I tried to replicate the problem described in [1] about logical
> replication from a v14 source DB to a v11 target.  It fails as
> described there; I've not yet tracked down why, but it looks like
> the v11 apply worker fails and closes the connection after sending
> CREATE_REPLICATION_SLOT.  The v14 walsender doesn't have a problem
> with that.  However, if I do the same thing using HEAD as the source,
> I see an assertion failure on the way out of the walsender.  The
> postmaster log shows

Gah, sorry, forgot about this one. I'll fix it / apply the fix.

> I guess this indicates that walsenders don't initialize their pgstats
> support soon enough?

It's more that we don't release slots early enough, I think.

Greetings,

Andres Freund

Re: fairywren is generating bogus BASE_BACKUP commands

[Andrew Dunstan]

On 2/3/22 20:51, Andres Freund wrote:
> Hi,
>
> On 2022-02-03 17:25:51 -0500, Andrew Dunstan wrote:
>> OK, I have all the pieces working and I know what I need to do to adapt
>> fairywren. The patch you provided is not necessary any more.
> Cool. Are you going to post that?



About the only thing missing in your recipe is this:


# force ucrt64 prove to use the ucrt64 perl rather than whatever is in
the path
sed -i 's,^#!perl,#!/ucrt64/bin/perl,' /ucrt64/bin/core_perl/prove


Given that, you don't need to set PERL, and configure can find the perl
to build against from the PATH.



>
>
>  Is there a reason to prefer ucrt64?
> There's a lot of oddities in the mingw64 target, due to targetting the much
> older C runtime library (lots of bugs, missing functionality). MSVC targets
> UCRT by default for quite a few years by now. Targetting msvcrt is basically
> on its way out from what I understand.


OK.


>> I think the next steps are:
>>
>>   * do those two reverts
>>   * adjust fairywren
>>   * get rid of perl2host
>>
>> At that stage jacana will no longer be able to run TAP tests. I can do
>> one of these:
> I guess because its install is too old?


Yeah. fairywren is now running with ucrt64-perl for TAP tests. 


>>   * disable the TAP tests on jacana
>>   * migrate jacana to msys2
>>   * kiss jacana goodbye.
> Having a non-server mingw animal seems like it could be useful (I think that's
> just Jacana), even if server / client versions of windows have grown
> closer. So I think an update to msys2 makes the most sense?


Working on that. There appear to be some issues with third party
libraries. I might need to rebuild libxml2 and zlib for example.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Possible to go without page headers?

[Peter Geoghegan]

On Mon, Feb 14, 2022 at 2:19 PM Tom Lane  wrote:
> No, at least not unless you plan to reimplement much of the WAL
> mechanism.  You do need at least an LSN in the right place.
> I kinda doubt that you can get away with ignoring checksumming,
> either.  On the whole, I think you'd be best off to use a standard
> page header; the amount you're saving by avoiding that will be
> minuscule, and the amount of work you cause for yourself probably
> not so much.

It isn't actually necessary for an index AM to use the standard
slotted page format to get the benefits that you mention, of course --
whether or not an index AM that uses standard page headers *also* uses
slotted pages with standard line pointers is a separate question. For
example, GIN posting tree pages don't use standard line pointers, but
still have a standard page header (and a generic GIN special area in
the opaque space).

I agree that it's hard to imagine that opting out of using the
standard page header format could ever make much sense. Principally
because the restrictions imposed on an index AM that uses the standard
page header format are very minimal, while the benefits are
substantial.

-- 
Peter Geoghegan

Re: Possible to go without page headers?

[David Steele]

On 2/14/22 16:19, Tom Lane wrote:

Chris Cleveland  writes:

Can I treat pages as just a flat, open 8k buffer and fill them with
arbitrary data?


No, at least not unless you plan to reimplement much of the WAL
mechanism.  You do need at least an LSN in the right place.
I kinda doubt that you can get away with ignoring checksumming,
either.  On the whole, I think you'd be best off to use a standard
page header; the amount you're saving by avoiding that will be
minuscule, and the amount of work you cause for yourself probably
not so much.

BTW, there are also tools such as pg_filedump that expect that index
pages can be identified by some sort of magic number kept in the
"special space" at the page tail.  You're not absolutely bound to make
that work, but you'll be cutting yourself off from some potentially
handy support.


You'll also get errors from external tools (like pgBackRest) that 
validate checksums and headers.


Regards,
-David

Re: Possible to go without page headers?

[Tom Lane]

Chris Cleveland  writes:
> Can I treat pages as just a flat, open 8k buffer and fill them with
> arbitrary data?

No, at least not unless you plan to reimplement much of the WAL
mechanism.  You do need at least an LSN in the right place.
I kinda doubt that you can get away with ignoring checksumming,
either.  On the whole, I think you'd be best off to use a standard
page header; the amount you're saving by avoiding that will be
minuscule, and the amount of work you cause for yourself probably
not so much.

BTW, there are also tools such as pg_filedump that expect that index
pages can be identified by some sort of magic number kept in the
"special space" at the page tail.  You're not absolutely bound to make
that work, but you'll be cutting yourself off from some potentially
handy support.

regards, tom lane

Better error message for unsupported replication cases

[Tom Lane]

In [1] there's a complaint that if you try to logically replicate
a partitioned table from v13-or-later to v12-or-earlier, you get
"table XXX not found on publisher", which is pretty confusing
because the publisher certainly does have such a table.  That
happens because fetch_remote_table_info is too aggressive about
filtering by relkind and doesn't see the relation at all.
c314c147c improved that, but it wasn't back-patched.  I propose
putting the attached into v10-v12.  Maybe the error message
could be bikeshedded ... is "non-table relation" terminology
that we use in user-facing messages?

regards, tom lane

[1] 
https://www.postgresql.org/message-id/CANhtRiamAgYt1A-Nh4%3DmU3E1UhG9XPgB%2BX6mW1DWqa93vUXW9A%40mail.gmail.com

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 2db88dc41a..61244e08fa 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -646,9 +646,10 @@ fetch_remote_table_info(char *nspname, char *relname,
 	WalRcvExecResult *res;
 	StringInfoData cmd;
 	TupleTableSlot *slot;
-	Oid			tableRow[2] = {OIDOID, CHAROID};
+	Oid			tableRow[3] = {OIDOID, CHAROID, CHAROID};
 	Oid			attrRow[4] = {TEXTOID, OIDOID, INT4OID, BOOLOID};
 	bool		isnull;
+	char		relkind;
 	int			natt;
 
 	lrel->nspname = nspname;
@@ -656,13 +657,12 @@ fetch_remote_table_info(char *nspname, char *relname,
 
 	/* First fetch Oid and replica identity. */
 	initStringInfo();
-	appendStringInfo(, "SELECT c.oid, c.relreplident"
+	appendStringInfo(, "SELECT c.oid, c.relreplident, c.relkind"
 	 "  FROM pg_catalog.pg_class c"
 	 "  INNER JOIN pg_catalog.pg_namespace n"
 	 "ON (c.relnamespace = n.oid)"
 	 " WHERE n.nspname = %s"
-	 "   AND c.relname = %s"
-	 "   AND c.relkind = 'r'",
+	 "   AND c.relname = %s",
 	 quote_literal_cstr(nspname),
 	 quote_literal_cstr(relname));
 	res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
@@ -683,6 +683,19 @@ fetch_remote_table_info(char *nspname, char *relname,
 	Assert(!isnull);
 	lrel->replident = DatumGetChar(slot_getattr(slot, 2, ));
 	Assert(!isnull);
+	relkind = DatumGetChar(slot_getattr(slot, 3, ));
+	Assert(!isnull);
+
+	/*
+	 * Newer PG versions allow things that aren't plain tables to appear in
+	 * publications.  We don't handle that in this version, but try to provide
+	 * a useful error message.
+	 */
+	if (relkind != RELKIND_RELATION)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot sync from non-table relation \"%s.%s\"",
+		nspname, relname)));
 
 	ExecDropSingleTupleTableSlot(slot);
 	walrcv_clear_result(res);

Possible to go without page headers?

[Chris Cleveland]

I'm writing an index access method with its own unique file format. It
involves storing large blobs that break across pages.

The file format itself doesn't need or use page headers. There's no need
for a checksum or to manage free space within the page.

Can I treat pages as just a flat, open 8k buffer and fill them with
arbitrary data?

The reason I ask is that I see some reference to an LSN, used to determine
when to dump a dirty buffer to disk, and don't know whether that is
actually required. I plan to write a large number of pages all at once and
I'm not yet quite sure how WAL logging will work. I also see some
suggestion that the vacuum process uses page headers, but I haven't quite
figured that out either.

Re: fixing bookindex.html bloat

[Peter Eisentraut]

On 14.02.22 18:31, Peter Eisentraut wrote:
Yeah, that is currently clearly wrong.  It appears I originally copied 
the wrong namespace declarations from examples that show how to 
customize the DocBook stylesheets, but those examples were apparently 
wrong or outdated in this respect.  It seems we also lack some namespace 
declarations altogether, as shown by your need to add it to 
stylesheet-html-common.xsl.  This appears to need some careful cleanup.


The attached patch cleans up the xhtml namespace declarations properly, 
I think.


For the xlink business, I don't have a better idea than you, so your 
workaround proposal seems fine.From 45d361e0bc7bd89b41880eb83cdccabf5626b71c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 14 Feb 2022 22:56:11 +0100
Subject: [PATCH] Fix XML namespace declarations

---
 doc/src/sgml/stylesheet-hh.xsl   | 4 +---
 doc/src/sgml/stylesheet-html-common.xsl  | 3 ++-
 doc/src/sgml/stylesheet-html-nochunk.xsl | 4 +---
 doc/src/sgml/stylesheet-text.xsl | 3 +--
 doc/src/sgml/stylesheet.xsl  | 3 +--
 5 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/stylesheet-hh.xsl b/doc/src/sgml/stylesheet-hh.xsl
index 1b1ab4bbe9..6f4b706dac 100644
--- a/doc/src/sgml/stylesheet-hh.xsl
+++ b/doc/src/sgml/stylesheet-hh.xsl
@@ -1,8 +1,6 @@
 
 http://www.w3.org/1999/XSL/Transform;
-version='1.0'
-xmlns="http://www.w3.org/TR/xhtml1/transitional;
-exclude-result-prefixes="#default">
+version='1.0'>
 
 http://docbook.sourceforge.net/release/xsl/current/htmlhelp/htmlhelp.xsl"/>
 
diff --git a/doc/src/sgml/stylesheet-html-common.xsl 
b/doc/src/sgml/stylesheet-html-common.xsl
index d9961089c6..96dd2cc038 100644
--- a/doc/src/sgml/stylesheet-html-common.xsl
+++ b/doc/src/sgml/stylesheet-html-common.xsl
@@ -4,7 +4,8 @@
 %common.entities;
 ]>
 http://www.w3.org/1999/XSL/Transform;
-version="1.0">
+version="1.0"
+xmlns="http://www.w3.org/1999/xhtml;>
 
 

Re: Fix overflow in justify_interval related functions

[Joseph Koshakow]

On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart  wrote:
> Makes sense.  So we could likely avoid it for justify_interval, but the
> others are at the mercy of the interval implementation.

I'm not entirely sure what you mean by "it", but for both
justify_interval and justify_days this commit throws an error if we
try to set the months field above 2^31.

> +SELECT justify_days(interval '2147483647 months 30 days');
> +ERROR:  interval out of range
> +SELECT justify_interval(interval '2147483647 months 30 days');
> +ERROR:  interval out of range

That's what these are testing.

- Joe Koshakow

Re: Observability in Postgres

[Nikolay Samokhvalov]

On Mon, Feb 14, 2022 at 10:15 Greg Stark  wrote:

>  
> For now my approach is to implement a background worker that listens
> on a new port and is basically its own small web server with shared
> memory access


This reminds me bg_mon (included into Spilo, docker image used by Zalando
operator for Postgres):

https://github.com/CyberDem0n/bg_mon

Re: sockaddr_un.sun_len vs. reality

[Thomas Munro]

On Tue, Feb 15, 2022 at 3:25 AM Robert Haas  wrote:
> It's not real clear to me that it's worth complicating the code to
> avoid a harmless compiler warning on an 11-year-old operating system
> with minimal real-world usage. On the other hand, if you really feel
> motivated to do something about it, I'm not here to argue. My one
> suggestion is that if you do add some strange incantation here along
> the lines you propose, you should at least add a comment explaining
> that it was done to suppress a warning on AIX 7.1. That way, there's a
> chance someone might be brave enough to try removing it in the future
> at such time as nobody cares about AIX 7.1 any more.

Just for the record, it's not just 11-year-old 7.1; 7.2 contains this
contradiction too, but our 7.2 animal doesn't complain because it's
using a different compiler.  I bet you one internet point 7.3 (which
just dropped in December, but isn't yet available for open source
hackers to scrounge an account on in the GCC farm) is the same.

By the way, speaking of open source on AIX, this distribution is
coming to an end:

http://www.bullfreeware.com/newsPage

Re: [PATCH] Add tests for psql tab completion

[Tom Lane]

[ Please keep the mailing list cc'd ]

Matheus Alcantara  writes:
> On Monday, February 14th, 2022 at 17:01, Tom Lane  wrote:
>> What exactly is the motivation for these particular tests?

> I was studying the source code and looking for projects that I could 
> contribute so I decided
> to start with tests, so I ran coverage and started with files that had little 
> coverage, realized
> that psql tab completion ones had little coverage so I decided to add some 
> tests, I tried to
> start with the simplest.

> I understand that the patch may not be as much of a need, I just wanted to 
> try and help with something.
> Do you think there would be other tests that should be done? I would like to 
> try to contribute.

There's certainly lots of places that could use more test coverage.
But I think that making a meaningful difference in tab-complete.c
would require writing test cases to hit most of the if-else branches,
which doesn't seem very profitable either in terms of test-writing
effort or in terms of the cycles that'd be spent on running those
tests forevermore.  We try to be thrifty about how much work is
done by check-world, because it's a real advantage for development
that that takes a small number of minutes and not hours.  I'm not
really seeing that covering more of tab-complete would buy much.

As for areas that *do* need more coverage, the first one that
I come across in looking through the coverage report is GIST
index build: gistbuild.c is only showing 45% coverage, and
gistbuildbuffers.c a fat zero [1].  We've looked at that before [2]
but not made much progress on developing an adequately cheap test
case.  Maybe you could pick up where that thread left off?  Or if that
doesn't seem interesting to you, there's lots of other possibilities.
I'd suggest getting some buy-in from this list on what to work on
before you start, though.

regards, tom lane

[1] https://coverage.postgresql.org/src/backend/access/gist/index.html

[2] 
https://www.postgresql.org/message-id/flat/10261.1588705157%40sss.pgh.pa.us#46b998e6585f0bf0fd7b75703b43decb

Re: PGroonga index-only scan problem with yesterday’s PostgreSQL updates

[Andrew Dunstan]

On 2/12/22 12:25, Tom Lane wrote:
>
> An even slicker answer would be to set up a PG buildfarm machine
> that, in addition to the basic tests, builds PGroonga against the
> new PG sources and runs your tests.  Andrew's machine "crake" does
> that for RedisFDW and BlackholeFDW, and the source code for at least
> the latter module is in the buildfarm client distribution.


It occurred to me that this wasn't very well documented, so I created
some docco:



cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Time to drop plpython2?

[Tom Lane]

Andres Freund  writes:
> On 2022-02-14 14:18:58 -0500, Tom Lane wrote:
>> Well, it's mid-February.  Do we have a python2-removal patch
>> that's ready to go?

> I can refresh mine. Iit might be good to first reapply
> f201da39edc - "Make configure prefer python3 to plain python."
> for a few days?

We could I guess, but does it really buy anything?  I'm sure that
some of the buildfarm still hasn't updated their Python installation,
but it'll be about the same failure we'd get from the final patch.

regards, tom lane

Assertion failure in pgstat_assert_is_up during walsender exit

[Tom Lane]

I tried to replicate the problem described in [1] about logical
replication from a v14 source DB to a v11 target.  It fails as
described there; I've not yet tracked down why, but it looks like
the v11 apply worker fails and closes the connection after sending
CREATE_REPLICATION_SLOT.  The v14 walsender doesn't have a problem
with that.  However, if I do the same thing using HEAD as the source,
I see an assertion failure on the way out of the walsender.  The
postmaster log shows

2022-02-14 15:18:29.890 EST [2922064] LOG:  starting logical decoding for slot 
"sub01"
2022-02-14 15:18:29.890 EST [2922064] DETAIL:  Streaming transactions 
committing after 0/FEA23B0, reading WAL from 0/FEA23B0.
2022-02-14 15:18:29.890 EST [2922064] STATEMENT:  START_REPLICATION SLOT 
"sub01" LOGICAL 0/0 (proto_version '1', publication_names '"pub01"')
2022-02-14 15:18:29.890 EST [2922064] LOG:  logical decoding found consistent 
point at 0/FEA23B0
2022-02-14 15:18:29.890 EST [2922064] DETAIL:  There are no running 
transactions.
2022-02-14 15:18:29.890 EST [2922064] STATEMENT:  START_REPLICATION SLOT 
"sub01" LOGICAL 0/0 (proto_version '1', publication_names '"pub01"')
2022-02-14 15:18:29.895 EST [2922066] LOG:  logical decoding found consistent 
point at 0/FEA2460
2022-02-14 15:18:29.895 EST [2922066] DETAIL:  There are no running 
transactions.
2022-02-14 15:18:29.895 EST [2922066] STATEMENT:  CREATE_REPLICATION_SLOT 
"sub01_129343_sync_129337" TEMPORARY LOGICAL pgoutput USE_SNAPSHOT
TRAP: FailedAssertion("pgstat_is_initialized && !pgstat_is_shutdown", File: 
"pgstat.c", Line: 5140, PID: 2922066)
postgres: walsender postgres [local] idle in 
transaction(ExceptionalCondition+0x7c)[0x95126c]
postgres: walsender postgres [local] idle in transaction[0x7a66dd]
postgres: walsender postgres [local] idle in transaction[0x7a68fb]
postgres: walsender postgres [local] idle in 
transaction(pgstat_report_replslot_drop+0x32)[0x7aad72]
postgres: walsender postgres [local] idle in transaction[0x7e7220]
postgres: walsender postgres [local] idle in 
transaction(ReplicationSlotCleanup+0xf1)[0x7e7561]
postgres: walsender postgres [local] idle in transaction[0x833982]
postgres: walsender postgres [local] idle in 
transaction(shmem_exit+0x85)[0x816195]
postgres: walsender postgres [local] idle in transaction[0x816277]
postgres: walsender postgres [local] idle in 
transaction(proc_exit+0x8)[0x816308]
postgres: walsender postgres [local] idle in 
transaction(PostgresMain+0x4b4)[0x83e2f4]
postgres: walsender postgres [local] idle in transaction[0x7b1872]
postgres: walsender postgres [local] idle in 
transaction(PostmasterMain+0xd2c)[0x7b282c]
postgres: walsender postgres [local] idle in transaction(main+0x1e9)[0x509489]
/lib64/libc.so.6(__libc_start_main+0xf3)[0x7fec53a86493]
postgres: walsender postgres [local] idle in transaction(_start+0x2e)[0x50976e]
2022-02-14 15:18:29.974 EST [2921719] LOG:  server process (PID 2922066) was 
terminated by signal 6: Aborted
2022-02-14 15:18:29.974 EST [2921719] LOG:  terminating any other active server 
processes

Stack trace looks like

#2  0x0095128b in ExceptionalCondition (
conditionName=conditionName@entry=0xabe798 "pgstat_is_initialized && 
!pgstat_is_shutdown", errorType=errorType@entry=0x9a4757 "FailedAssertion", 
fileName=fileName@entry=0xabe554 "pgstat.c", 
lineNumber=lineNumber@entry=5140) at assert.c:69
#3  0x007a66dd in pgstat_assert_is_up () at pgstat.c:5140
#4  0x007a68fb in pgstat_assert_is_up () at pgstat.c:3244
#5  pgstat_send (msg=, len=) at pgstat.c:3228
#6  0x007aad72 in pgstat_report_replslot_drop (
slotname=slotname@entry=0x7fec53654320 "sub01_129343_sync_129337")
at pgstat.c:1948
#7  0x007e7220 in ReplicationSlotDropPtr (slot=0x7fec53654308)
at slot.c:696
#8  0x007e7561 in ReplicationSlotCleanup () at slot.c:547
#9  0x00833982 in ProcKill (code=, arg=)
at proc.c:839
#10 ProcKill (code=, arg=) at proc.c:804
#11 0x00816195 in shmem_exit (code=code@entry=0) at ipc.c:272
#12 0x00816277 in proc_exit_prepare (code=code@entry=0) at ipc.c:194
#13 0x00816308 in proc_exit (code=code@entry=0) at ipc.c:107
#14 0x0083e2f4 in PostgresMain (dbname=, 

I guess this indicates that walsenders don't initialize their pgstats
support soon enough?

regards, tom lane

[1] 
https://www.postgresql.org/message-id/CANhtRiamAgYt1A-Nh4%3DmU3E1UhG9XPgB%2BX6mW1DWqa93vUXW9A%40mail.gmail.com

Re: Time to drop plpython2?

[Andres Freund]

Hi,

On 2022-02-14 14:18:58 -0500, Tom Lane wrote:
> Well, it's mid-February.  Do we have a python2-removal patch
> that's ready to go?

I can refresh mine. Iit might be good to first reapply
f201da39edc - "Make configure prefer python3 to plain python."
for a few days?

Greetings,

Andres Freund

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Maciek Sakrejda]

Andrew made a good case above for avoiding LOG:

>I do think we should be wary of any name starting with "LOG", though.
>Long experience tells us that's something that confuses users when it
refers to the WAL.

Re: generic plans and "initial" pruning

[Robert Haas]

On Sun, Feb 13, 2022 at 4:55 PM David Rowley  wrote:
> FWIW, that would remove the whole point in init run-time pruning.  The
> reason I made two phases of run-time pruning was so that we could get
> away from having the init plan overhead of nodes we'll never need to
> scan.  If we wanted to show the (never executed) scans in EXPLAIN then
> we'd need to do the init plan part and allocate all that memory
> needlessly.

Interesting. I didn't realize that was why it had ended up like this.

> I understood at the time it was just the EXPLAIN output that you had
> concerns with. I thought that was just around the lack of any display
> of the condition we used for pruning.

That was part of it, but I did think it was surprising that we didn't
print anything at all about the nodes we pruned, too. Although we're
technically iterating over the PlanState, from the user perspective it
feels like you're asking PostgreSQL to print out the plan - so it
seems weird to have nodes in the Plan tree that are quietly omitted
from the output. That said, perhaps in retrospect it's good that it
ended up as it did, since we'd have a lot of trouble printing anything
sensible for a scan of a table that's since been dropped.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Observability in Postgres

[Greg Stark]

So I've been dealing a lot with building and maintaining dashboards
for (fleets of) Postgres servers. And it's a pain. I have a few
strongly held ideas about where the pain points are and what the right
ways to tackle them are. Some of which are going to be controversial I
think...

The state of the art is pg_exporter which is a separate client that
connects to the database and runs SQL queries to extract database
metrics. The pain points I've seen are:

1) The exporter isn't reliable when things are failing. If your
clients can't connect the exporter also can't connect leading to data
gaps in your metrics for precisely the time windows where you need
data. This can happen to connection exhaustion, xid wraparound, or
even something as simple as someone taking an exclusive lock on
something used in the sql queries.

2) SQL connections are tied to specific databases within a cluster.
Making it hard to get data for all your databases if you have more
than one. The exporter needs to reconnect to each database.

3) The exporter needs to listen on a different port from the
postmaster. Making it necessary to write software to manage the
mapping from server port to exporter port and that's left to the
end-user as it varies from site to site.

4) The queries are customizable (the built-in ones don't exhaustively
exporter postgres's metrics). As a result there's no standard
dashboard that will work on any site out of the box. Moreover issue
(3) also makes it impossible to implement one that works properly.

5) data needs to be marshaled from shared memory into SQL and then
read by the client and re-emitted in the metric format. The double
processing requires writing SQL queries very carefully to avoid losing
fidelity for things like LSN positions, xids, etc. Moreover the
latency and gathering data from multiple SQL queries results in
metrics that are often out of sync with each other making them hard to
interpret.

All this said, I think we should have a component in Postgres that
reads from the stats data directly and outputs metrics in standard
metrics format directly. This would probably take the form of a
background worker with a few tricky bits.

This would mean there would be a standard official set of metrics
available that a standard dashboard could rely on to be present at any
site and it would be reliable if the SQL layer isn't functioning due
to lack of connections or xid wraparound or locking issues.

The elephant in the room is that issue (3) requires a bit of sleight
of hand. Ideally I would want it to be listening on the same ports as
the database. That means having the postmaster recognize metric
requests and hand them to the metrics background worker instead of a
backend. I'm not sure people are going to be ok with that

For now my approach is to implement a background worker that listens
on a new port and is basically its own small web server with shared
memory access. This ignores issue (3) and my hope is that when we have
some experience with this approach we'll see how reliable it is and
how comfortable we are with the kind of hacking in postmaster it would
take to fix it. Fwiw I do think this is an important issue and not one
that we can ignore indefinitely.

There is another elephant in the room (it's a big room) which is that
this all makes sense for stats data. It doesn't make much sense for
data that currently lives in pg_class, pg_index, etc. In other words
I'm mostly solving (2) by ignoring it and concentrating on stats data.

I haven't settled on a good solution for that data. I vaguely lean
towards saying that the volatile metrics in those tables should really
live in stats or at least be mirrored there. That makes a clean
definition of what Postgres thinks a metric is and what it thinks
catalog data is. But I'm not sure that will really work in practice.
In particular I think it's likely we'll need to get catalog data from
every database anyways, for example to label things like tables with
better labels than oids.

This work is being funded by Aiven which is really interested in
improving observability and integration between Postgres and other
open source cloud software.

-- 
greg

Re: buildfarm warnings

[Andrew Dunstan]

On 2/12/22 16:13, Justin Pryzby wrote:
> Is there any check for warnings from new code, other than those buildfarm
> members with -Werror ?


I had forgotten about this :-) but a few years ago I provided a
check_warnings setting (and a --check-warnings command line parameter).
It's processed if set in the main build steps ("make", "make contrib"
and "make_test_modules") as well as some modules that check external
software. But it hasn't been widely advertised so it's probably in
hardly any use, if at all.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Teach pg_receivewal to use lz4 compression

[Robert Haas]

On Fri, Feb 11, 2022 at 10:52 PM Michael Paquier  wrote:
> > And, on a related note, Michael, do you plan to get something
> > committed here?
>
> Apart from f79962d, ba5 and 50e1441, I don't think that there was
> something left to do for this thread.  Perhaps I am missing something?

Oh, my mistake. I didn't realize you'd already committed it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Robert Haas]

On Mon, Feb 14, 2022 at 1:58 PM Bruce Momjian  wrote:
> > I think we have consensus on STRATEGY. I'm not sure if we have
> > consensus on what the option values should be. If we had an option to
> > use fs-based cloning, that would also need to issue a checkpoint,
> > which makes me think that CHECKPOINT is not the best name.
>
> I think if we want LOG, it has tob e WAL_LOG instead of just LOG.  Was
> there discussion that the user _has_ to specify and option instead of
> using a default?  That doesn't seem good.

I agree. I think we can set a default, which can be either whatever we
think will be best on average, or maybe it can be conditional based on
the database size or something.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [PATCH] Add tests for psql tab completion

[Tom Lane]

Matheus Alcantara  writes:
> I'm attaching a patch that add some new test cases for tab completion of psql.

What exactly is the motivation for these particular tests?

I believe that most of tab-complete.c is already covered, outside
of the giant if-else chain at the heart of psql_completion().
It's hard to summon interest in trying to cover every branch of
that chain; but if we're to add coverage of just a few more,
which ones and why?  The ones you've chosen to hit in this
patch don't seem especially interesting.

regards, tom lane

Re: Time to drop plpython2?

[Tom Lane]

I wrote:
> Peter Eisentraut  writes:
>> On 12.01.22 19:49, Tom Lane wrote:
> Anyway, getting back to the point: I think we should notify the
> owners ASAP and set a 30-day deadline.

>> Sure, let's do that.  I don't have a buildfarm animal these days, so I'm 
>> not on that list, so it would be great if you could do that

> Done.  I told them "mid February", so we can plan on say the 15th
> as the target date for pushing a patch.

Well, it's mid-February.  Do we have a python2-removal patch
that's ready to go?

regards, tom lane

Re: Fix overflow in justify_interval related functions

[Nathan Bossart]

On Mon, Feb 14, 2022 at 01:55:56PM -0500, Tom Lane wrote:
> Nathan Bossart  writes:
>> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
>>> +SELECT justify_hours(interval '2147483647 days 24 hrs');
>>> +ERROR:  interval out of range
> 
>> The docs [0] claim that the maximum value for interval is 178 million
>> years, but this test case is only ~6 million.  Should we instead rework the
>> logic to avoid overflow for this case?
> 
> I think the docs are misleading you on this point.  The maximum
> value of the months part of an interval is 2^31 months or
> about 178Myr, but what we're dealing with here is days, which
> likewise caps at 2^31 days.  justify_hours is not chartered
> to transpose up to months, so it can't avoid that limit.

Makes sense.  So we could likely avoid it for justify_interval, but the
others are at the mercy of the interval implementation.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Bruce Momjian]

On Mon, Feb 14, 2022 at 12:27:10PM -0500, Robert Haas wrote:
> On Mon, Feb 14, 2022 at 11:26 AM Dilip Kumar  wrote:
> > So do we have consensus to use (STRATEGY = LOG/CHECKPOINT or do we
> > think that keeping it bool i.e. Is LOG_COPIED_BLOCKS a better option?
> > Once we have consensus on this I will make this change and
> > documentation as well along with the other changes suggested by
> > Robert.
> 
> I think we have consensus on STRATEGY. I'm not sure if we have
> consensus on what the option values should be. If we had an option to
> use fs-based cloning, that would also need to issue a checkpoint,
> which makes me think that CHECKPOINT is not the best name.

I think if we want LOG, it has tob e WAL_LOG instead of just LOG.  Was
there discussion that the user _has_ to specify and option instead of
using a default?  That doesn't seem good.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Re: Fix overflow in justify_interval related functions

[Tom Lane]

Nathan Bossart  writes:
> On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
>> +SELECT justify_hours(interval '2147483647 days 24 hrs');
>> +ERROR:  interval out of range

> The docs [0] claim that the maximum value for interval is 178 million
> years, but this test case is only ~6 million.  Should we instead rework the
> logic to avoid overflow for this case?

I think the docs are misleading you on this point.  The maximum
value of the months part of an interval is 2^31 months or
about 178Myr, but what we're dealing with here is days, which
likewise caps at 2^31 days.  justify_hours is not chartered
to transpose up to months, so it can't avoid that limit.

regards, tom lane

Re: Non-decimal integer literals

[Christoph Berg]

Re: Peter Eisentraut
> This adds support in the lexer as well as in the integer type input
> functions.
> 
> Those core parts are straightforward enough, but there are a bunch of other
> places where integers are parsed, and one could consider in each case
> whether they should get the same treatment, for example the replication
> syntax lexer, or input function for oid, numeric, and int2vector.

One thing I always found weird is that timeline IDs appear most
prominently as hex numbers in WAL filenames, but they are printed as
decimal in the log ("new timeline id nn"), and have to be specified as
decimal in recovery_target_timeline.

Perhaps both these could make use of 0xhex numbers as well.

Christoph

Re: Fix overflow in justify_interval related functions

[Nathan Bossart]

On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
> +SELECT justify_hours(interval '2147483647 days 24 hrs');
> +ERROR:  interval out of range

The docs [0] claim that the maximum value for interval is 178 million
years, but this test case is only ~6 million.  Should we instead rework the
logic to avoid overflow for this case?

[0] https://www.postgresql.org/docs/devel/datatype-datetime.html

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Mark all GUC variable as PGDLLIMPORT

[Robert Haas]

On Mon, Feb 14, 2022 at 12:55 PM Tom Lane  wrote:
> Robert Haas  writes:
> > An alternative rule which would dodge that particular issue would be
> > to just slap PGDLLIMPORT on every global variable in every header
> > file. That would arguably be a simpler rule, though it means even more
> > PGDLLIMPORT declarations floating around.
>
> Yeah, if the objective is "level playing field for Windows",
> then it's hard to avoid the conclusion that we should just do that.
> Again, I've never had an objection to that as the end result ---
> I just wish that we could get the toolchain to do it for us.
> But if we can't, we can't.

100% agreed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: warn if GUC set to an invalid shared library

[Maciek Sakrejda]

On Wed, Feb 9, 2022 at 5:58 PM Justin Pryzby  wrote:
> FYI, it has said "while..." and hasn't said "guc" since the 2nd revision of 
> the
> patch.

The v3-0001 attached above had "while... for GUC..."--sorry I wasn't clear.

In v4, the message looks fine to me for shared_preload_libraries
(except there is a doubled "is"). However, I also get the message for
a simple SET with local_preload_libraries:

postgres=# set local_preload_libraries=xyz;
WARNING:  could not access file "xyz"
HINT:  The server will fail to start with the existing configuration.
If it is is shut down, it will be necessary to manually edit the
postgresql.auto.conf file to allow it to start.
SET

I'm not familiar with that setting (reading the docs, it's like a
non-superuser session_preload_libraries for compatible modules?), but
given nothing is being persisted here with ALTER SYSTEM, this seems
incorrect.

Changing session_preload_libraries emits a similar warning:

postgres=# set session_preload_libraries = foo;
WARNING:  could not access file "$libdir/plugins/foo"
HINT:  New sessions will fail with the existing configuration.
SET

This is also not persisted, so I think this is also incorrect, right?
(I'm not sure what setting session_preload_libraries without an ALTER
ROLE or ALTER DATABASE accomplishes, given a new session is required
for the change to take effect, but I thought I'd point this out.) I'm
guessing this may be due to trying to have the warning for ALTER ROLE?

postgres=# alter role bob set session_preload_libraries = foo;
WARNING:  could not access file "$libdir/plugins/foo"
HINT:  New sessions will fail with the existing configuration.
ALTER ROLE

This is great. Ideally, we'd qualify this with "New sessions for
user..." or "New sessions for database..." but given you get the
warning right after running the relevant command, maybe that's clear
enough.

> $ ./tmp_install/usr/local/pgsql/bin/postgres -D 
> src/test/regress/tmp_check/data -clogging_collector=on
> 2022-02-09 19:53:48.034 CST postmaster[30979] FATAL:  could not access file 
> "a": No such file or directory
> 2022-02-09 19:53:48.034 CST postmaster[30979] CONTEXT:  while loading shared 
> libraries for setting "shared_preload_libraries"
> from 
> /home/pryzbyj/src/postgres/src/test/regress/tmp_check/data/postgresql.auto.conf:3
> 2022-02-09 19:53:48.034 CST postmaster[30979] LOG:  database system is shut 
> down
>
> Maybe it's enough to show the GucSource rather than file:line...

This is great. I think the file:line output is helpful here.

Thanks,
Maciek

Re: bailing out in tap tests nearly always a bad idea

[Tom Lane]

Andres Freund  writes:
> Even just getting rid of the "Tests were run but no plan was declared and
> done_testing() was not seen." noise would be helpful. So I think using a fatal
> error routine that forced a failure to be recognized via ok(0, 'fatal error')
> and then does done_testing() would be better...

Maybe we could do something in an END block provided by Utils.pm?
I still think that insisting that people avoid die() is going to
be annoying.

regards, tom lane

Re: Mark all GUC variable as PGDLLIMPORT

[Tom Lane]

Robert Haas  writes:
> An alternative rule which would dodge that particular issue would be
> to just slap PGDLLIMPORT on every global variable in every header
> file. That would arguably be a simpler rule, though it means even more
> PGDLLIMPORT declarations floating around.

Yeah, if the objective is "level playing field for Windows",
then it's hard to avoid the conclusion that we should just do that.
Again, I've never had an objection to that as the end result ---
I just wish that we could get the toolchain to do it for us.
But if we can't, we can't.

regards, tom lane

Re: fixing bookindex.html bloat

[Andres Freund]

Hi,

On 2022-02-14 18:31:25 +0100, Peter Eisentraut wrote:
> > The reason that we end up with so many more xmlns:xlink is just that without
> > our customization there ends up being a single
> > http://www.w3.org/1999/xlink; class="index">
> > and then everything below that doesn't need the xmlns:xlink anymore. But
> > because stylesheet-html-common.xsl emits the div, the xmlns:xlink is emitted
> > for each element that autoidx.xsl has "control" over.
> > 
> > Waiting for docbook to fix this seems a bit futile, I eventually found a
> > bugreport about this, from 2016: 
> > https://sourceforge.net/p/docbook/bugs/1384/
> > 
> > But we can easily reduce the "impact" of the issue, by just adding a single
> > xmlns:xlink to , which is sufficient to convince xsltproc
> > to not repeat it.
> 
> I haven't fully wrapped my head around this.  I tried adding xlink to our
> own exclude-result-prefixes, but that didn't seem to have the right effect.

It can't, because it's not one of our stylesheets that causes the xlink: stuff
to be included. It's autoidx.xls - just adding xlink to autoidx's
exclude-result-prefixes fixes the problem "properly", but we can't really
modify it.

The reason adding xmlns:xlink to our div (or even higher up) helps is that
then nodes below it don't need to include it again (when output by autoidx),
which drastically reduces the number of xmlns:xlink. So it's just a somewhat
ugly workaround, but for >100kB it seems better than the alternative.

Greetings,

Andres Freund

Re: bailing out in tap tests nearly always a bad idea

[Robert Haas]

On Sun, Feb 13, 2022 at 6:33 PM Tom Lane  wrote:
> Huh, doesn't Test::More already provide a sane way to do this?
> If not, why isn't die() good enough?  (I don't think you can
> realistically expect to prohibit die() anywhere in the TAP tests.)

+1 for die. There's very little reason to use BAIL_OUT. If we agree
that we need a wrapper for die() for some reason, I can live with
that, too, but die is simple and familiar to perl programmers.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: bailing out in tap tests nearly always a bad idea

[Andres Freund]

Hi,

On 2022-02-14 16:08:42 +, Dagfinn Ilmari Mannsåker wrote:
> die()-ing is the correct way to abort a single test script.

There's really no way to nice way to abort without the "Dubious, test returned
255 (wstat 65280, 0xff00)" stuff?

Even just getting rid of the "Tests were run but no plan was declared and
done_testing() was not seen." noise would be helpful. So I think using a fatal
error routine that forced a failure to be recognized via ok(0, 'fatal error')
and then does done_testing() would be better...


> t/die-immediately.t  aieee at t/die-immediately.t line 1.
> t/die-immediately.t  Dubious, test returned 255 (wstat 65280, 0xff00)
> No subtests run

Hm. Looks different when a test is including our test helpers.

t/000_die_pg_test_utils.pl .. Dubious, test returned 25 (wstat 6400, 0x1900)
No subtests run
t/000_die_test_more.pl .. error at t/000_die_test_more.pl line 2.
t/000_die_test_more.pl .. Dubious, test returned 255 (wstat 65280, 0xff00)
No subtests run

So I think we broke something... I think it's the log file redirection stuff
in INIT.

Greetings,

Andres Freund

Re: Mark all GUC variable as PGDLLIMPORT

[Robert Haas]

On Mon, Feb 14, 2022 at 12:34 PM Tom Lane  wrote:
> I think you are attributing straw-man positions to me.  What I'd actually
> *like* is some solution that has the effect of (1) without having to mark
> up our code with a bunch of Microsoft-isms.  However I don't know how to
> do that, and I do agree that (2), (3), and (4) are not better than (1).

OK, that sounds like we might be making some progress toward a useful agreement.

> There are some technical issues though:
>
> * AFAICS, the patch of record doesn't actually do (1), it does something
> else that adds yet more new notation.  The cfbot says it fails, too.

That sounds like a problem. Should be fixable.

> * If we institute a policy that all GUCs should be PGDLLIMPORT'd,
> how will we enforce that new patches follow that?  I don't want to be
> endlessly going back and adding forgotten PGDLLIMPORT markers.

It seems to me that it would be no different from bumping catversion
or updating nodefuncs.c: yes, it will get forgotten sometimes, but
hopefully people will mostly get used to it just like they do with
dozens of other PG-specific coding rules. I don't see that it's likely
to generate more than a handful of commits per year, so it doesn't
really seem worth worrying about to me, but YMMV. Maybe it's possible
to write a perl script to verify it, although it seems like it might
be complicated to code that up.

> * There's a moderately sizable subset of GUCs where the underlying
> variable is not visible at all because it's static in guc.c.
> Typically this is because that variable is only used for display
> and there's an assign hook that stores the real data somewhere else.
> I suppose what we want in such cases is for the "somewhere else"
> to be PGDLLIMPORT'd, but in a lot of cases those variables are also
> static in some other module.  Does this proposal include exporting
> variables that currently aren't visible to extensions at all?
> I'm a little resistant to that.  I can buy making sure that Windows
> has a level playing field, but that's as far as I want to go.

I can live with that. If someone complains about those variables being
static-to-file instead of globally visible, we can address that
complaint on its merits when it is presented.

An alternative rule which would dodge that particular issue would be
to just slap PGDLLIMPORT on every global variable in every header
file. That would arguably be a simpler rule, though it means even more
PGDLLIMPORT declarations floating around.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Mark all GUC variable as PGDLLIMPORT

[Robert Haas]

On Mon, Feb 14, 2022 at 12:25 PM Chapman Flack  wrote:
> On 02/14/22 11:43, Robert Haas wrote:
> > On Mon, Feb 14, 2022 at 10:38 AM Tom Lane  wrote:
> >> Robert Haas  writes:
> >>> I don't particularly like Chapman's solution,
> > ... and (3) is an attempt at compromise that is nobody's first choice.
>
> Ok, I guess that's )sniffle( a pretty fair way of putting it.

I mean I like it better than Tom does ... I just think it's a
complicated work around for a problem we don't really need to have.

> My reading of past discussions on the list suggest that stronger
> encapsulation and API delineation have advocates in some quarters,
> so I tried to accommodate that in what I proposed. It does, for example,
> avoid exposing the 'real' value of a GUC to writing by a buggy
> or compromised extension.
>
> I'll be first to agree what I proposed isn't beautiful, but it might be
> that a round or two of improvement by somebody smarter than me could lead
> to something possibly preferable to PGDLLIMPORT-everywhere /when measured
> against certain objectives/, like encapsulation.
>
> So maybe this is in part a discussion about what the weights should be
> on those various objectives.

Keep in mind that there's nothing being set in stone here. Applying
PGDLLIMPORT to all GUCs across the board does not foreclose putting
some other system that restricts symbol visibility in the future. But
in the present, there is clearly no approach to the symbol visibility
problem that is fully baked or enjoys consensus. Yet, there is clearly
consensus that not being able to access GUCs in Windows extensions is
a problem. There must be like six different threads with people
complaining about that, and in every case the suggested solution is to
just add PGDLLIMPORT for crying out loud.

If in the future there is a consensus to restrict symbol visibility in
order to achieve some agreed-upon amount of encapsulation, then we can
do that then. Extension authors may not like it much, but every
serious extension author understands that the PostgreSQL code base
needs to keep moving forward, and that this will sometimes require
them to adjust for new major versions. This would be a bigger
adjustment than many, but I have confidence that if the benefits are
worthwhile, people will cope with it and adjust their extensions to
adhere to whatever new rules we put in place.

But between now and then, refusing to apply PGDLLIMPORT to a basically
random subset of GUCs is just annoying people without any compensating
benefit.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Mark all GUC variable as PGDLLIMPORT

[Tom Lane]

Robert Haas  writes:
> Hmm, I guess I'd need to know who those people are in order to be able
> to review their comments. I don't *like* the extra notational cruft,
> but applying it inconsistently isn't better than being consistent. As
> I see it, we have four choices: (1) apply PGDLLIMPORT markings
> relatively broadly so that people can get extensions to work on
> Windows, (2) continue to apply them inconsistently, thus slightly
> reducing notational clutter at the cost of breaking lots of extensions
> on Windows, (3) put some complex system in place like what Chapman
> proposes and get all extension authors to adopt it, and (4) remove the
> Windows port. To the best of my current knowledge, everyone other than
> you prefers (1), you prefer (2) or (4), and (3) is an attempt at
> compromise that is nobody's first choice.

I think you are attributing straw-man positions to me.  What I'd actually
*like* is some solution that has the effect of (1) without having to mark
up our code with a bunch of Microsoft-isms.  However I don't know how to
do that, and I do agree that (2), (3), and (4) are not better than (1).

There are some technical issues though:

* AFAICS, the patch of record doesn't actually do (1), it does something
else that adds yet more new notation.  The cfbot says it fails, too.

* If we institute a policy that all GUCs should be PGDLLIMPORT'd,
how will we enforce that new patches follow that?  I don't want to be
endlessly going back and adding forgotten PGDLLIMPORT markers.

* There's a moderately sizable subset of GUCs where the underlying
variable is not visible at all because it's static in guc.c.
Typically this is because that variable is only used for display
and there's an assign hook that stores the real data somewhere else.
I suppose what we want in such cases is for the "somewhere else"
to be PGDLLIMPORT'd, but in a lot of cases those variables are also
static in some other module.  Does this proposal include exporting
variables that currently aren't visible to extensions at all?
I'm a little resistant to that.  I can buy making sure that Windows
has a level playing field, but that's as far as I want to go.

regards, tom lane

Re: fixing bookindex.html bloat

[Peter Eisentraut]

On 13.02.22 21:16, Andres Freund wrote:

The reason for the two xmlns= are different. The
xmlns="http://www.w3.org/1999/xhtml; is afaict caused by confusion on our
part.

Some of our stylesheets use
xmlns="http://www.w3.org/TR/xhtml1/transitional;
others use
xmlns="http://www.w3.org/1999/xhtml;

It's noteworthy that the docbook xsl stylesheets end up with
http://www.w3.org/1999/xhtml;>
so it's a bit pointless to reference http://www.w3.org/TR/xhtml1/transitional
afaict.

Adding xmlns="http://www.w3.org/1999/xhtml; to stylesheet-html-common.xsl gets
rid of xmlns="http://www.w3.org/TR/xhtml1/transitional; in bookindex specific
content.

Changing stylesheet.xsl from transitional to http://www.w3.org/1999/xhtml gets
rid of xmlns="http://www.w3.org/TR/xhtml1/transitional; in navigation/footer.

Of course we should likely change all http://www.w3.org/TR/xhtml1/transitional
references, rather than just the one necessary to get rid of the xmlns= spam.


Yeah, that is currently clearly wrong.  It appears I originally copied 
the wrong namespace declarations from examples that show how to 
customize the DocBook stylesheets, but those examples were apparently 
wrong or outdated in this respect.  It seems we also lack some namespace 
declarations altogether, as shown by your need to add it to 
stylesheet-html-common.xsl.  This appears to need some careful cleanup.



The reason that we end up with so many more xmlns:xlink is just that without
our customization there ends up being a single
http://www.w3.org/1999/xlink; class="index">
and then everything below that doesn't need the xmlns:xlink anymore. But
because stylesheet-html-common.xsl emits the div, the xmlns:xlink is emitted
for each element that autoidx.xsl has "control" over.

Waiting for docbook to fix this seems a bit futile, I eventually found a
bugreport about this, from 2016: https://sourceforge.net/p/docbook/bugs/1384/

But we can easily reduce the "impact" of the issue, by just adding a single
xmlns:xlink to , which is sufficient to convince xsltproc
to not repeat it.


I haven't fully wrapped my head around this.  I tried adding xlink to 
our own exclude-result-prefixes, but that didn't seem to have the right 
effect.

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Robert Haas]

On Mon, Feb 14, 2022 at 11:26 AM Dilip Kumar  wrote:
> So do we have consensus to use (STRATEGY = LOG/CHECKPOINT or do we
> think that keeping it bool i.e. Is LOG_COPIED_BLOCKS a better option?
> Once we have consensus on this I will make this change and
> documentation as well along with the other changes suggested by
> Robert.

I think we have consensus on STRATEGY. I'm not sure if we have
consensus on what the option values should be. If we had an option to
use fs-based cloning, that would also need to issue a checkpoint,
which makes me think that CHECKPOINT is not the best name.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Mark all GUC variable as PGDLLIMPORT

[Chapman Flack]

On 02/14/22 11:43, Robert Haas wrote:
> On Mon, Feb 14, 2022 at 10:38 AM Tom Lane  wrote:
>> Robert Haas  writes:
>>> I don't particularly like Chapman's solution,
> ... and (3) is an attempt at compromise that is nobody's first choice.

Ok, I guess that's )sniffle( a pretty fair way of putting it.

> ... (3) put some complex system in place like what Chapman
> proposes and get all extension authors to adopt it,

By the same token, I don't think it would entail anything like a big
flag day for extension authors. Provided no one proposes immediately
to /remove/ PGDLLIMPORT from things that now have it, the effect would
be more that the next time an extension author comes hat-in-hand
asking for a new PGDLLIMPORT because a thing won't build on Windows,
the answer can be "here, we have this API you can use for that now."

My reading of past discussions on the list suggest that stronger
encapsulation and API delineation have advocates in some quarters,
so I tried to accommodate that in what I proposed. It does, for example,
avoid exposing the 'real' value of a GUC to writing by a buggy
or compromised extension.

I'll be first to agree what I proposed isn't beautiful, but it might be
that a round or two of improvement by somebody smarter than me could lead
to something possibly preferable to PGDLLIMPORT-everywhere /when measured
against certain objectives/, like encapsulation.

So maybe this is in part a discussion about what the weights should be
on those various objectives.

Regards,
-Chap

Re: automatically generating node support functions

[Tom Lane]

Peter Eisentraut  writes:
> What do people think about this patch now?

I'm in favor of moving forward with this.  I do not like the
libclang-based approach that Andres was pushing, because of the
jump in developer tooling requirements that it'd cause.

Eyeballing the patch a bit, I do have some comments:

* It's time for action on the business about extracting comments
from the to-be-deleted code.

* The Perl script is kind of under-commented for my taste.
It lacks a copyright notice, too.

* In the same vein, I should not have to reverse-engineer what
the available pg_node_attr() properties are or do.  Perhaps they
could be documented in the comment for the pg_node_attr macro
in nodes.h.

* Maybe the generated file names could be chosen less opaquely,
say ".funcs" and ".switch" instead of ".inc1" and ".inc2".

* I don't understand why there are changes in the #include
lists in copyfuncs.c etc?

* I think that more thought needs to be put into the format
of the *nodes.h struct declarations, because I fear pgindent
is going to make a hash of what you've done here.  When we
did similar stuff in the catalog headers, I think we ended
up moving a lot of end-of-line comments onto their own lines.

* I assume the pg_config_manual.h changes are not meant for
commit?

regards, tom lane

Re: [PATCH] Add reloption for views to enable RLS

[Christoph Heiss]

Hi all,

again, many thanks for the reviews and testing!

On 2/4/22 17:09, Laurenz Albe wrote:

I also see no reason to split a small patch like this into three parts.
I've split it into the three unrelated parts (code, docs, tests) to ease 
review, but I happily carry it as one patch too.



In the attached, I dealt with the above and went over the comments.
How do you like it?


That is really nice, I used it to base v6 on.

On 2/9/22 17:40, walt...@technowledgy.de wrote:
Ah, good find. In that case, I suggest to change the docs slightly to 
say that the schema will not be checked.


In one place it's described as "it will cause all access to the 
underlying tables to be checked as ..." which is fine, I think. But in 
another place it's "access to tables, functions and *other objects* 
referenced in the view, ..." which is misleading

I removed the reference to "other objects" for now in v6.

I agree that the name "security_invoker" is suggestive of SECURITY 
INVOKER

in CREATE FUNCTION, but the behavior is different.
Perhaps the solution is as simple as choosing a different name that does
not prompt this association, for example "permissions_invoker".


Yes, given that there is not much that can be done about the 
functionality anymore, a different name would be better. This would also 
avoid the implicit "if security_invoker=false, the view behaves like 
SECURITY DEFINER" association, which is also clearly wrong. And this 
assumption is actually what made me think the chained views example was 
somehow off.


I am not convinced "permissions_invoker" is much better, though. The 
difference between SECURITY INVOKER and SECURITY DEFINER is invoker vs 
definer... where, I think, we need something else to describe what we 
currently have and what the patch provides.


Maybe we can look at it from the other perspective: Both ways of 
operating keep the CURRENT_USER the same, pretty much like what we 
understand "security invoker" should do. The difference, however, is the 
current default in which the permissions are checked with the view 
*owner*. Let's treat this difference as the thing that can be set: 
security_owner=true|false. Or run_as_owner=true|false.


xxx_owner=true would be the default and xxx_owner=false could be set 
explicitly to get the behavior we are looking for in this patch?


I'm not sure if an option which is on by default would be best, IMHO. I 
would rather have an off-by-default option, so that you explicitly have 
to turn *on* that behavior rather than turning *off* the current.


[ Pretty much bike-shedding here, but if the agreement comes to one of 
"xxx_owner" I won't mind it either. ]


My best suggestions is maybe something like run_as_invoker=t|f, but that 
would probably raise the same "invoker vs definer" association.


I left it for now as-is.


I guess more documentation how this works would be a good idea.
[...] but since it exposes this
in new ways, it might as well clarify how all this works.


I tried to clarify this situation in the documentation in a concise 
matter, I'd appreciate further feedback on that.


Thanks,
Christoph HeissFrom 7d8f8e1cb27a3d22d049a5d820ddd66ec77a3297 Mon Sep 17 00:00:00 2001
From: Christoph Heiss 
Date: Mon, 14 Feb 2022 17:54:35 +0100
Subject: [PATCH v6 1/1] Add new boolean reloption "security_invoker" to views

When this reloption is set to true, all permissions on the underlying
objects will be checked against the invoking user rather than the view
owner, as is currently implemented.

Signed-off-by: Christoph Heiss 
Co-Author: Laurenz Albe 
Discussion: https://postgr.es/m/b66dd6d6-ad3e-c6f2-8b90-47be773da240%40cybertec.at
Signed-off-by: Christoph Heiss 
---
 doc/src/sgml/ref/alter_view.sgml  | 12 +++-
 doc/src/sgml/ref/create_view.sgml | 73 ++-
 src/backend/access/common/reloptions.c| 11 
 src/backend/nodes/copyfuncs.c |  1 +
 src/backend/nodes/equalfuncs.c|  1 +
 src/backend/nodes/outfuncs.c  |  1 +
 src/backend/nodes/readfuncs.c |  1 +
 src/backend/optimizer/plan/subselect.c|  1 +
 src/backend/optimizer/prep/prepjointree.c |  1 +
 src/backend/rewrite/rewriteHandler.c  | 20 +--
 src/backend/utils/cache/relcache.c| 63 ++-
 src/include/nodes/parsenodes.h|  1 +
 src/include/utils/rel.h   | 11 
 src/test/regress/expected/create_view.out | 46 +++---
 src/test/regress/expected/rowsecurity.out | 65 +++-
 src/test/regress/sql/create_view.sql  | 22 ++-
 src/test/regress/sql/rowsecurity.sql  | 44 ++
 17 files changed, 313 insertions(+), 61 deletions(-)

diff --git a/doc/src/sgml/ref/alter_view.sgml b/doc/src/sgml/ref/alter_view.sgml
index 98c312c5bf..8bdc90a5a1 100644
--- a/doc/src/sgml/ref/alter_view.sgml
+++ b/doc/src/sgml/ref/alter_view.sgml
@@ -156,7 +156,17 @@ ALTER VIEW [ IF EXISTS ] name RESET
 
  
   Changes 

Re: Mark all GUC variable as PGDLLIMPORT

[Robert Haas]

On Mon, Feb 14, 2022 at 10:38 AM Tom Lane  wrote:
> Robert Haas  writes:
> > I don't particularly like Chapman's solution, but given that you've
> > repeatedly blocked every effort to just apply PGDLLIMPORT markings
> > across the board, I'm not sure what the realistic alternative is.
>
> You do realize that I just have one vote in these matters?  If I'm
> outvoted then so be it.  The impression I have though is that a
> number of other people don't like the extra notational cruft either.

Hmm, I guess I'd need to know who those people are in order to be able
to review their comments. I don't *like* the extra notational cruft,
but applying it inconsistently isn't better than being consistent. As
I see it, we have four choices: (1) apply PGDLLIMPORT markings
relatively broadly so that people can get extensions to work on
Windows, (2) continue to apply them inconsistently, thus slightly
reducing notational clutter at the cost of breaking lots of extensions
on Windows, (3) put some complex system in place like what Chapman
proposes and get all extension authors to adopt it, and (4) remove the
Windows port. To the best of my current knowledge, everyone other than
you prefers (1), you prefer (2) or (4), and (3) is an attempt at
compromise that is nobody's first choice.

If that is correct, then I think we should do (1). If it's incorrect
then I think we should do our best to find a choice other than (2)
that does attract a consensus. The current situation, which is (2),
must be the worst of all possible options because it manages to bother
the people who dislike the clutter AND ALSO bother the people who want
to have their extensions work on Windows. Any other choice has a
chance of reaching a state where only one of those groups of people is
annoyed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Sun, Feb 13, 2022 at 10:12 AM Dilip Kumar  wrote:
>
> On Sat, Feb 12, 2022 at 2:38 AM Alvaro Herrera  
> wrote:

> > It seems you're thinking deciding what to do based on an option that
> > gets a boolean argument.  But what about making the argument be an enum?
> > For example
> >
> > CREATE DATABASE ... WITH (STRATEGY = LOG);  -- default if option is 
> > omitted
> > CREATE DATABASE ... WITH (STRATEGY = CHECKPOINT);
> >
> > So the user has to think about it in terms of some strategy to choose,
> > rather than enabling or disabling some flag with nontrivial
> > implications.
>
>
> Yeah I think being explicit about giving the strategy to the user
> looks like a better option.  Now they can choose whether they want it
> to create using WAL log or using CHECKPOINT.  Otherwise, if we give a
> flag then we will have to give an explanation that if they choose not
> to WAL log then we will have to do a checkpoint internally.  So I
> think giving LOG vs CHECKPOINT as an explicit option looks better to
> me.

So do we have consensus to use (STRATEGY = LOG/CHECKPOINT or do we
think that keeping it bool i.e. Is LOG_COPIED_BLOCKS a better option?
Once we have consensus on this I will make this change and
documentation as well along with the other changes suggested by
Robert.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: bailing out in tap tests nearly always a bad idea

[Dagfinn Ilmari Mannsåker]

Andres Freund  writes:

> Hi,
>
> On 2022-02-13 18:32:59 -0500, Tom Lane wrote:
>> Andres Freund  writes:
>> > Best with a
>> > central function signalling fatal error, rather than individual uses of die
>> > or such.
>>
>> Huh, doesn't Test::More already provide a sane way to do this?
>
> I looked, and didn't see anything. But I'm not a perl person, so I might just
> have missed something.
>
>> If not, why isn't die() good enough?  (I don't think you can
>> realistically expect to prohibit die() anywhere in the TAP tests.)

die()-ing is the correct way to abort a single test script.

> The output of dying isn't great either:
>
> t/000_fail.pl  Dubious, test returned 25 (wstat 6400, 
> 0x1900)
> No subtests run
>
> it'd be nicer if that that showed the actual reason for failing, rather than
> the unhelpful "Dubious, test returned" stuff.

It does output the die() message, but it's on stderr, while the harness
output is on stdout, so redirections might interfere.

$ echo 'use Test::More; die "aieee";' > t/die-immediately.t
$ echo 'use Test::More; pass "yay"; die "aieee";' > t/die-after-success.t
$ prove --quiet 
t/die-after-success.t .. 1/? aieee at t/die-after-success.t line 1.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 1.
t/die-after-success.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
All 1 subtests passed
t/die-immediately.t  aieee at t/die-immediately.t line 1.
t/die-immediately.t  Dubious, test returned 255 (wstat 65280, 0xff00)
No subtests run
  
> Greetings,
>
> Andres Freund

- ilmari

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Dilip Kumar]

On Mon, Feb 14, 2022 at 9:17 PM Ashutosh Sharma  wrote:
>
>
> Is it possible to see the WAL size generated by these two statements:
> UPDATE 70% of the tuple in the base table (dirty 70% of the shared
> buffers) && CREATE database using template DB (Actual test target).
> Just wanted to know if it can exceed the max_wal_size of 64GB.

I think we already know the wal size generated by creating a db with
an old and new approach.  With the old approach it is just one WAL log
and with the new approach it is going to log every single block of the
database.  Yeah the updating 70% of the database could have some
impact but for verification purposes I tested without the update and
still the create db with WAL log is taking almost the same time.  But
anyway when I test next time I will verify again that no force
checkpoint is triggered.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: refactoring basebackup.c

[Robert Haas]

On Sat, Feb 12, 2022 at 1:01 AM Shinoda, Noriyoshi (PN Japan FSIP)
 wrote:
> Thank you for developing a great feature.
> The current help message shown below does not seem to be able to specify the 
> 'client-' or 'server-' for lz4 compression.
>  --compress = {[{client, server}-]gzip, lz4, none}[:LEVEL]
>
> The attached small patch fixes the help message as follows:
>  --compress = {[{client, server}-]{gzip, lz4}, none}[:LEVEL]

Hmm. After studying this a bit more closely, I think this might
actually need a bit more revision than what you propose here. In most
places, we use vertical bars to separate alternatives:

  -X, --wal-method=none|fetch|stream

But here, we're using commas in some places and the word "or" in one
case as well:

  -Z, --compress={[{client,server}-]gzip,lz4,none}[:LEVEL] or [LEVEL]

We're also not consistently using braces for grouping, which makes the
order of operations a bit unclear, and it makes no sense to put
brackets around LEVEL when it's the only thing that's part of that
alternative.

A more consistent way of writing the supported syntax would be like this:

  -Z, --compress={[{client|server}-]{gzip|lz4}}[:LEVEL]|LEVEL|none}

I would be somewhat inclined to leave the level-only variant
undocumented and instead write it like this:

  -Z, --compress={[{client|server}-]{gzip|lz4}}[:LEVEL]|none}

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Database-level collation version tracking

[Peter Eisentraut]

On 14.02.22 10:14, Julien Rouhaud wrote:

Do you plan to send a rebased version of the ICU default collation
soon or should I start looking at the current v4?


I will send an updated patch in the next few days.

Re: [Proposal] Fully WAL logged CREATE DATABASE - No Checkpoints

[Ashutosh Sharma]

Hi Dilip,

On Sun, Feb 13, 2022 at 12:04 PM Dilip Kumar  wrote:
>
> On Sun, Feb 13, 2022 at 10:12 AM Dilip Kumar  wrote:
> >
>
> I have done performance testing with different template DB sizes and
> different amounts of dirty shared buffers and I think as expected the
> bigger the dirty shared buffer the checkpoint approach becomes costly
> and OTOH the larger the template DB size the WAL log approach takes
> more time.
>
> I think it is very common to have larger shared buffers and of course,
> if somebody has configured such a large shared buffer then a good % of
> it will be dirty most of the time.  So IMHO in the future, the WAL log
> approach is going to be more usable in general.  However, this is just
> my opinion, and others may have completely different thoughts and
> anyhow we are keeping options for both the approaches so no worry.
>
> Next, I am planning to do some more tests, where we are having pgbench
> running and concurrently we do CREATEDB maybe every 1 minute and see
> what is the CREATEDB time as well as what is the impact on pgbench
> performance.  Because currently I have only measured CREATEDB time but
> we must be knowing the impact of createdb on the other system as well.
>
> Test setup:
> max_wal_size=64GB
> checkpoint_timeout=15min
> - CREATE base TABLE of size of Shared Buffers
> - CREATE template database and table in it of varying sizes (as per test)
> - CHECKPOINT (write out dirty buffers)
> - UPDATE 70% of tuple in base table (dirty 70% of shared buffers)
> - CREATE database using template db. (Actual test target)
>
> test1:
> 1 GB shared buffers, template DB size = 6MB, dirty shared buffer=70%
> Head: 2341.665 ms
> Patch: 85.229 ms
>
> test2:
> 1 GB shared buffers, template DB size = 1GB, dirty shared buffer=70%
> Head: 4044 ms
> Patch: 8376 ms
>
> test3:
> 8 GB shared buffers, template DB size = 1GB, dirty shared buffer=70%
> Head: 21398 ms
> Patch: 9834 ms
>
> test4:
> 8 GB shared buffers, template DB size = 10GB, dirty shared buffer=95%
> Head: 38574 ms
> Patch: 77160 ms
>
> test4:
> 32 GB shared buffers, template DB size = 10GB, dirty shared buffer=70%
> Head: 47656 ms
> Patch: 79767 ms
>

Is it possible to see the WAL size generated by these two statements:
UPDATE 70% of the tuple in the base table (dirty 70% of the shared
buffers) && CREATE database using template DB (Actual test target).
Just wanted to know if it can exceed the max_wal_size of 64GB. Also,
is it possible to try with minimal wal_level? Sorry for asking you
this, I could try it myself but I don't have any high level system to
try it.

--
With Regards,
Ashutosh Sharma.

Re: Mark all GUC variable as PGDLLIMPORT

[Tom Lane]

Robert Haas  writes:
> I don't particularly like Chapman's solution, but given that you've
> repeatedly blocked every effort to just apply PGDLLIMPORT markings
> across the board, I'm not sure what the realistic alternative is.

You do realize that I just have one vote in these matters?  If I'm
outvoted then so be it.  The impression I have though is that a
number of other people don't like the extra notational cruft either.

regards, tom lane

Re: Mark all GUC variable as PGDLLIMPORT

[Robert Haas]

On Sun, Feb 13, 2022 at 3:17 PM Tom Lane  wrote:
> > ...
> >   ObserveTypedConfigValue("log_statement_sample_rate",
> > , _changed, SAMPRATE_CHANGED);
> > ...
>
> > and will be subscribed to have the native-format value stored into samprate,
> > and SAMPRATE_CHANGED ORed into gucs_changed, whenever the value changes.
>
>
> That seems like about 10X more complexity than is warranted,
> not only in terms of the infrastructure required, but also in
> the intellectual complexity around "just when could that value
> change?"

I agree in the sense that I believe we should $SUBJECT rather than
fooling around with this. It is completely understandable that
extensions want to know the value of GUCs, and not just as strings,
and doing $SUBJECT would be by far the easiest way of accomplishing
that. I'm sure Andres is right when he says that there are cases where
not exposing symbols could improve the generated machine code, but I'm
also pretty sure that we just have to live with the cost when it's
core GUCs that we're talking about. It's just unrealistic to suppose
that extensions are not going to care.

But if we're not going to do that, then I don't see why Chapman's
proposal is overly complex. It seems completely understandable that if
(some_guc_var != last_observed_some_guc_var) { ...adapt accordingly...
} feels like an OK thing to do but if you have to make a function call
every time then it seems too expensive. Imagine a background worker
that has to do a bunch of extra setup every time some GUC changes, and
every iteration of the main loop just wants to check whether it's
changed, and the main loop could iterate very quickly in some cases. I
wouldn't want to worry about the cost of a function call on every trip
through the loop. Maybe it would be trivial in practice, but who
knows?

I don't particularly like Chapman's solution, but given that you've
repeatedly blocked every effort to just apply PGDLLIMPORT markings
across the board, I'm not sure what the realistic alternative is. It
doesn't seem fair to argue, on the one hand, that we can't just do
what I believe literally every other hacker on the project wants, and
that on the other hand, it's also unacceptable to add complexity to
work around the problem you've created by blocking that proposal every
time it's been raised year after year. It's really pretty frustrating
to me that we haven't just done the obvious thing here years ago. The
time we've spent arguing about it could have been better spent on just
about anything else, with absolutely zero harm to the project.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: sockaddr_un.sun_len vs. reality

[Tom Lane]

Thomas Munro  writes:
> On Mon, Feb 14, 2022 at 7:19 PM Tom Lane  wrote:
>> I'm leaning to adding a compile-time clamp on the value,
>> that is
>> 
>>  unp->sun_len = Min(sizeof(struct sockaddr_un),
>> (1 << (sizeof(unp->sun_len) * 8)) - 1);

> Any system that has sun_len, and has expanded sun_path so that the
> struct size doesn't fit in sun_len, must surely be ignoring sun_len
> but retaining it for binary compatibility.  Otherwise there would be
> no way to use the extra bytes of sun_path!  It's tempting to suggest
> setting sun_len to zero in this case...

Yeah, I thought about doing that or just skipping the assignment
altogether.  However, we'd need just as much code, because the
change would have to be conditional on more or less this same
computation as to whether sizeof(struct sockaddr_un) fits into
the field.

regards, tom lane

Re: Merging statistics from children instead of re-sampling everything

[Tomas Vondra]

On 2/14/22 11:22, Andrey V. Lepikhov wrote:

On 2/11/22 20:12, Tomas Vondra wrote:



On 2/11/22 05:29, Andrey V. Lepikhov wrote:

On 2/11/22 03:37, Tomas Vondra wrote:

That being said, this thread was not really about foreign partitions,
but about re-analyzing inheritance trees in general. And sampling
foreign partitions doesn't really solve that - we'll still do the
sampling over and over.

IMO, to solve the problem we should do two things:
1. Avoid repeatable partition scans in the case inheritance tree.
2. Avoid to re-analyze everything in the case of active changes in 
small subset of partitions.


For (1) i can imagine a solution like multiplexing: on the stage of 
defining which relations to scan, group them and prepare parameters 
of scanning to make multiple samples in one shot.
I'm not sure I understand what you mean by multiplexing. The term 
usually means "sending multiple signals at once" but I'm not sure how 
that applies to this issue. Can you elaborate?


I suppose to make a set of samples in one scan: one sample for plane 
table, another - for a parent and so on, according to the inheritance 
tree. And cache these samples in memory. We can calculate all parameters 
of reservoir method to do it.




I doubt keeping the samples just in memory is a good solution. Firstly, 
there's the question of memory consumption. Imagine a large partitioned 
table with 1-10k partitions. If we keep a "regular" sample (30k rows) 
per partition, that's 30M-300M rows. If each row needs 100B, that's 
3-30GB of data.


Sure, maybe we could keep smaller per-partition samples, large enough to 
get the merged sample of 30k row. But then you can also have higher 
statistics target values, the rows can be larger, etc.


So a couple of GB per inheritance tree can easily happen. And this data 
may not be used all that often, so keeping it in memory may be wasteful.


But maybe you have an idea how to optimize sizes per-partition samples? 
In principle we need


  30k * size(partition) / size(total)

for each partition, but the trouble is partitions may be detached, data 
may be deleted from some partitions, etc.


Also, what would happen after a restart? If we lose the samples, we'll 
have to resample everything anyway - and after a restart the system is 
usually fairly busy, so that's not a great timing.


So IMHO the samples need to be serialized, in some way.


sample might be used for estimation of clauses directly.
You mean, to use them in difficult cases, such of estimation of grouping 
over APPEND ?


That's one example, yes. But the sample might be used even to estimate 
complex conditions on a single partition (there's plenty of stuff we 
can't estimate from MCV/histogram).


But it requires storing the sample somewhere, and I haven't found a 
good and simple way to do that. We could serialize that into bytea, or 
we could create a new fork, or something, but what should that do with 
oversized attributes (how would TOAST work for a fork) and/or large 
samples (which might not fit into 1GB bytea)? 
This feature looks like meta-info over a database. It can be stored in 
separate relation. It is not obvious that we need to use it for each 
relation, for example, with large samples. I think, it can be controlled 
by a table parameter.




Well, a separate catalog is one of the options. But I don't see how that 
deals with large samples, etc.



regards

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

Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

[Ashutosh Sharma]

Here are few comments:

+/*
+ * Verify the authenticity of the given raw WAL record.
+ */
+Datum
+pg_verify_raw_wal_record(PG_FUNCTION_ARGS)
+{


Do we really need this function? I see that whenever the record is
read, we verify it. So could there be a scenario where any of these
functions would return an invalid WAL record?

--

Should we add a function that returns the pointer to the first and
probably the last WAL record in the WAL segment? This would help users
to inspect the wal records in the entire wal segment if they wish to
do so.

--

+PG_FUNCTION_INFO_V1(pg_get_raw_wal_record);
+PG_FUNCTION_INFO_V1(pg_get_first_valid_wal_record_lsn);
+PG_FUNCTION_INFO_V1(pg_verify_raw_wal_record);
+PG_FUNCTION_INFO_V1(pg_get_wal_record_info);
+PG_FUNCTION_INFO_V1(pg_get_wal_records_info);

I think we should allow all these functions to be executed in wait and
*nowait* mode. If a user specifies nowait mode, the function should
return if no WAL data is present, rather than waiting for new WAL data
to become available, default behaviour could be anything you like.

--

+Datum
+pg_get_wal_records_info(PG_FUNCTION_ARGS)
+{
+#define PG_GET_WAL_RECORDS_INFO_COLS 10


We could probably have another variant of this function that would
work even if the end pointer is not specified, in which case the
default end pointer would be the last WAL record in the WAL segment.
Currently it mandates the use of an end pointer which slightly reduces
flexibility.

--

+
+/*
+ * Get the first valid raw WAL record lsn.
+ */
+Datum
+pg_get_first_valid_wal_record_lsn(PG_FUNCTION_ARGS)


I think this function should return a pointer to the nearest valid WAL
record which can be the previous WAL record to the LSN entered by the
user or the next WAL record. If a user unknowingly enters an lsn that
does not exist then in such cases we should probably return the lsn of
the previous WAL record instead of hanging or waiting for the new WAL
record to arrive.

--

Another important point I would like to mention here is - have we made
an attempt to ensure that we try to share as much of code with
pg_waldump as possible so that if any changes happens in the
pg_waldump in future it gets applied here as well and additionally it
will also reduce the code duplication.

I haven't yet looked into the code in detail. I will have a look at it
asap. thanks.

--
With Regards,
Ashutosh Sharma.

On Sat, Feb 12, 2022 at 5:03 PM Bharath Rupireddy
 wrote:
>
> On Thu, Feb 10, 2022 at 9:55 PM Peter Geoghegan  wrote:
> >
> > On Sun, Feb 6, 2022 at 7:45 AM Robert Haas  wrote:
> > > For what it's worth, I am generally in favor of having something like
> > > this in PostgreSQL. I think it's wrong of us to continue assuming that
> > > everyone has command-line access. Even when that's true, it's not
> > > necessarily convenient. If you choose to use a relational database,
> > > you may be the sort of person who likes SQL. And if you are, you may
> > > want to have the database tell you what's going on via SQL rather than
> > > command-line tools or operating system utilities. Imagine if we didn't
> > > have pg_stat_activity and you had to get that information by running a
> > > separate binary. Would anyone like that? Why is this case any
> > > different?
> >
> > +1. An SQL interface is significantly easier to work with. Especially
> > because it can use the built-in LSN type, pg_lsn.
> >
> > I don't find the slippery slope argument convincing. There aren't that
> > many other things that are like pg_waldump, but haven't already been
> > exposed via an SQL interface. Offhand, I can't think of any.
>
> On Sat, Feb 12, 2022 at 4:03 AM Andrew Dunstan  wrote:
> >
> > Almost completely off topic, but this reminded me of an incident about
> > 30 years ago at my first gig as an SA/DBA. There was an application
> > programmer who insisted on loading a set of values from a text file into
> > a temp table (it was Ingres, anyone remember that?). Why? Because he
> > knew how to write "Select * from mytable order by mycol" but didn't know
> > how to drive the Unix sort utility at the command line. When I was
> > unable to restrain myself from smiling at this he got very angry and
> > yelled at me loudly.
> >
> > So, yes, some people do like SQL and hate the command line.
>
> Thanks a lot  for the comments. I'm looking forward to the review of
> the latest v4 patches posted at [1].
>
> [1] 
> https://www.postgresql.org/message-id/CALj2ACUS9%2B54QGPtUjk76dcYW-AMKp3hPe-U%2BpQo2-GpE4kjtA%40mail.gmail.com
>
> Regards,
> Bharath Rupireddy.

Re: Identify missing publications from publisher while create/alter subscription.

[Ashutosh Sharma]

Thanks for working on my review comments. I'll take a look at the new
changes and let you know my comments, if any. I didn't get a chance to
check it out today as I was busy reviewing some other patches, but
I'll definitely take a look at the new patch in a day or so and let
you know my feedback.

--
With Regards,
Ashutosh Sharma.

On Sun, Feb 13, 2022 at 7:34 PM vignesh C  wrote:
>
> On Fri, Feb 11, 2022 at 7:14 PM Ashutosh Sharma  wrote:
> >
> > I have spent little time looking at the latest patch. The patch looks
> > to be in good shape as it has already been reviewed by many people
> > here, although I did get some comments. Please take a look and let me
> > know your thoughts.
> >
> >
> > +   /* Try to connect to the publisher. */
> > +   wrconn = walrcv_connect(sub->conninfo, true, sub->name, );
> > +   if (!wrconn)
> > +   ereport(ERROR,
> > +   (errmsg("could not connect to the publisher: %s", err)));
> >
> > I think it would be good to also include the errcode
> > (ERRCODE_CONNECTION_FAILURE) here?
>
> Modified
>
> > --
> >
> > @@ -514,6 +671,8 @@ CreateSubscription(ParseState *pstate,
> > CreateSubscriptionStmt *stmt,
> >
> > PG_TRY();
> > {
> > +   check_publications(wrconn, publications, 
> > opts.validate_publication);
> > +
> >
> >
> > Instead of passing the opts.validate_publication argument to
> > check_publication function, why can't we first check if this option is
> > set or not and accordingly call check_publication function? For other
> > options I see that it has been done in the similar way for e.g. check
> > for opts.connect or opts.refresh or opts.enabled etc.
>
> Modified
>
> > --
> >
> > Above comment also applies for:
> >
> > @@ -968,6 +1130,8 @@ AlterSubscription(ParseState *pstate,
> > AlterSubscriptionStmt *stmt,
> > replaces[Anum_pg_subscription_subpublications - 1] = true;
> >
> > update_tuple = true;
> > +   connect_and_check_pubs(sub, stmt->publication,
> > +  opts.validate_publication);
> >
>
> Modified
>
> > --
> >
> > + 
> > +  When true, the command verifies if all the specified publications
> > +  that are being subscribed to are present in the publisher and 
> > throws
> > +  an error if any of the publication doesn't exist. The default is
> > +  false.
> >
> > publication -> publications (in the 4th line : throw an error if any
> > of the publication doesn't exist)
> >
> > This applies for both CREATE and ALTER subscription commands.
>
> Modified
>
> Thanks for the comments, the attached v14 patch has the changes for the same.
>
> Regard,s
> Vignesh