Re: Adding CI to our tree
Hi,
On October 2, 2021 1:18:38 PM PDT, Daniel Gustafsson wrote:
>> On 2 Oct 2021, at 22:01, Andres Freund wrote:
>> On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:
>>> Same, and for my case I run several CI jobs to compile/test against
>>> different
>>> OpenSSL versions etc.
>>
>> On that note: Did you do this for windows? If so, I'd rather not figure that
>> out myself...
>
>Not with Cirrus, I've been using Appveyor for Windows and they provide 1.0.2 -
>3.0.0 which can easily set in config.pl with for example:
>
>$config->{openssl} = 'C:\OpenSSL-v111-Win64';
Got the build part working (although the state of msvc compatible openssl
distribution on windows seems a bit scary). However the ssl tests don't fully
succeed:
https://cirrus-ci.com/task/6264790323560448?logs=ssl#L655
I didn't see code in the bf client code running the test so perhaps that's not
too surprising :/
Did you run those tests on windows?
Regards,
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
Andrew Dunstan writes: > On 10/2/21 5:03 PM, Tom Lane wrote: >> IIUC, the only problem for a non-updated animal would be that it'd >> run the test twice? Or would it actually fail? If the latter, >> we'd need to sit on the patch rather longer. > The patch removes test.sh, so yes it would break. Maybe we could leave test.sh in place for awhile? I'd rather not cause a flag day for buildfarm owners. (Also, how do we see this working in the back branches?) regards, tom lane
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On 10/2/21 5:03 PM, Tom Lane wrote: > Andrew Dunstan writes: >> I haven't looked at the patch closely yet, but from a buildfarm POV I >> think the only thing that needs to be done is to inhibit the buildfarm >> client module if the TAP tests are present. The buildfarm code that runs >> TAP tests should automatically detect and run the new test. >> I've just counted and there are 116 animals reporting check-pg_upgrade, >> so we'd better put that out pronto. It's a little early but I'll try to >> push out a release containing code for it on Monday or Tuesday (it's a >> one line addition). > IIUC, the only problem for a non-updated animal would be that it'd > run the test twice? Or would it actually fail? If the latter, > we'd need to sit on the patch rather longer. > > The patch removes test.sh, so yes it would break. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Enabling deduplication with system catalog indexes
On Fri, Oct 1, 2021 at 2:35 PM Bossart, Nathan wrote: > On 9/30/21, 3:44 PM, "Peter Geoghegan" wrote: > > I will commit this patch in a few days, barring objections. > > +1 Okay, pushed. Thanks -- Peter Geoghegan
Re: 002_types.pl fails on some timezones on windows
Thomas Munro writes: > On Sun, Oct 3, 2021 at 11:26 AM Tom Lane wrote: >> I'd still defend our exception for Central America: CLDR maps that >> to Guatemala which seems pretty random, even if they haven't observed >> DST there for a few years. For the rest of it, though, "we follow CLDR" >> has definitely got a lot of attraction. Actually ... digging in the archives, the reason we have a special case for Central America is that there was a user complaint about the previous mapping to CST6CDT: https://www.postgresql.org/message-id/flat/1316149023380-4809498.post%40n5.nabble.com CST6CDT was *way* wrong, because it implies USA DST rules, so the complaint was well-founded. I wrote in that thread: > I think we ought to map "Central America Standard Time" to plain CST6. > (Or we could map to one of America/Costa_Rica, America/Guatemala, > America/El_Salvador, etc, but that seems more likely to offend people in > the other countries than provide any additional precision.) However, if we can cite CLDR as authority, I see no reason why America/Guatemala should be any more offensive than any of the other fairly-arbitrary choices CLDR has made. None of those zones have observed DST for a decade or more, so at least in recent years it wouldn't make any difference anyway. So, I'm now sold on just making all our mappings match CLDR. I'll do that in a couple of days if I don't hear objections. > It would be interesting to know if that idea of matching BCP47 locale > names to territories could address that. Perhaps we should get that > modern-locale-name patch first (I think I got stuck on "let's kill off > old Windows versions so we can use this", due to confusing versioning > and a lack of a guiding policy on our part, but I think I should just > propose something), and then revisit this? That seems like potentially a nice long-term solution, but it doesn't sound likely to be back-patchable. So I'd like to get the existing data in as good shape as we can before we go looking for a replacement mechanism. regards, tom lane
Re: 002_types.pl fails on some timezones on windows
Andres Freund writes: > On October 2, 2021 3:26:35 PM PDT, Tom Lane wrote: >> However, the example >> we started the thread with is that Andres thought "Greenwich Standard >> Time" would get him UTC, or at least something a lot less oddball than >> what he got. > FWIW, that was just the default on those machines (which in turn seems to be > the default of some containers Microsoft distributes), not something I > explicitly chose. So *somebody* thought it was an unsurprising default ... regards, tom lane
Re: Query that will not run a ValuePerCall SRF to completion?
Chapman Flack writes: > I'm having difficulty coming up with a query that actually doesn't > run the SRF to completion. >From memory, nodeFunctionscan always populates the tuplestore immediately. I've looked into changing that but not got it done. If you write the function in the targetlist, ie select srf(...) limit N; I think it will act more like you expect. regards, tom lane
Query that will not run a ValuePerCall SRF to completion?
Hi,
I am trying to improve (i.e. have any at all) test coverage of the
ExprContextCallback for a ValuePerCall SRF function handler.
I'm having difficulty coming up with a query that actually doesn't
run the SRF to completion.
The form I've been trying looks like
SELECT *
FROM
executeSelectToRecords('SELECT * FROM generate_series(1,100)')
AS (thing int)
LIMIT 10;
but even that query calls executeSelectToRecords for all 100 rows
and then shows me ten of them, and the callback isn't tested.
Is there a way to write a simple query that won't run the SRF to
completion?
Thanks!
Regards,
-Chap
Re: Better context for "TAP tests not enabled" error message
> On 3 Oct 2021, at 00:39, Kevin Burke wrote: > Updated patch that removes the "Maybe" Thanks, I’ll take care of this tomorrow along with Rachels patch. ./daniel
Re: 002_types.pl fails on some timezones on windows
On Sun, Oct 3, 2021 at 11:26 AM Tom Lane wrote: > > Eyeballing these, three look strange to me in a list of otherwise > > city-based names: Pacific/Guam (instead of Port Moresby, capital of > > PNG which apparently shares zone rules with the territory of Guam) and > > Pacific/Samoa (country name instead of its capital Apia; the city > > avoids any potential confusion with American Samoa which is on the > > other side of the date line) and then "CET", an abbreviation. > > Oooh. Looking closer, I see that the Windows zone is defined as > > which makes it *definitely* Pacific/Apia ... Pacific/Samoa is a > link to Pacific/Pago_Pago which is in American Samoa, at UTC-11. > So our mapping was kind of okay up till 2011 when Samoa decided > they wanted to be on the other side of the date line, but now > it's wrong as can be. Ooops. Hah. That's a *terrible* link to have. > I'd still defend our exception for Central America: CLDR maps that > to Guatemala which seems pretty random, even if they haven't observed > DST there for a few years. For the rest of it, though, "we follow CLDR" > has definitely got a lot of attraction. The one change that makes me > nervous is adopting Europe/Berlin for "W. Europe Standard Time", > on account of the flak Paul Eggert just got from trying to make a > somewhat-similar change :-(. It would be interesting to know if that idea of matching BCP47 locale names to territories could address that. Perhaps we should get that modern-locale-name patch first (I think I got stuck on "let's kill off old Windows versions so we can use this", due to confusing versioning and a lack of a guiding policy on our part, but I think I should just propose something), and then revisit this? > (If you don't read the tz mailing list > you may not be aware of that particular tempest in a teapot, but he > tried to merge a bunch of zones into Europe/Berlin, and there were > a lot of complaints. Some from me.) I don't follow the list but there was a nice summary in LWN: "A fork for the time-zone database?". From the peanut gallery, I thought it was a bit of a double standard, considering the rejection of that idea of yours about getting rid of longitude-based pre-standard times on data stability grounds, and a lot less justifiable. I hope there isn't a fork.
Re: 002_types.pl fails on some timezones on windows
Hi, On October 2, 2021 3:26:35 PM PDT, Tom Lane wrote: >However, the example >we started the thread with is that Andres thought "Greenwich Standard >Time" would get him UTC, or at least something a lot less oddball than >what he got. FWIW, that was just the default on those machines (which in turn seems to be the default of some containers Microsoft distributes), not something I explicitly chose. - Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: 002_types.pl fails on some timezones on windows
Thomas Munro writes: > On Sun, Oct 3, 2021 at 9:42 AM Tom Lane wrote: >> * Now-documented decision to map "Greenwich Standard Time" >> to Europe/London, not Atlantic/Reykjavik as they have it. > Hmm. It's hard to pick a default from that set of merged zones, but > the funny thing about this choice is that Europe/London is the one > Olson zone that it's sure *not* to be, because then your system would > be using that other name, IIUC. Agreed, this choice is definitely formally wrong. However, the example we started the thread with is that Andres thought "Greenwich Standard Time" would get him UTC, or at least something a lot less oddball than what he got. But wait a minute ... looking into the tzdb sources, I find that Iceland hasn't observed DST since 1968, and tzdb spells their zone abbreviation as "GMT" since then. That means that Atlantic/Reykjavik is actually a way better approximation to "plain GMT" than Europe/London is. Maybe there is some method in CLDR's madness here. >> * The miscellaneous deltas shown in the attached diff, which in >> many cases boil down to "we chose the first name mentioned for the >> zone, while CLDR did something else". I felt that our historical >> mappings of these cases weren't wrong enough to justify any >> political flak I might take for changing them. OTOH, maybe we >> should just say "we follow CLDR" and be done with it. > Eyeballing these, three look strange to me in a list of otherwise > city-based names: Pacific/Guam (instead of Port Moresby, capital of > PNG which apparently shares zone rules with the territory of Guam) and > Pacific/Samoa (country name instead of its capital Apia; the city > avoids any potential confusion with American Samoa which is on the > other side of the date line) and then "CET", an abbreviation. Oooh. Looking closer, I see that the Windows zone is defined as which makes it *definitely* Pacific/Apia ... Pacific/Samoa is a link to Pacific/Pago_Pago which is in American Samoa, at UTC-11. So our mapping was kind of okay up till 2011 when Samoa decided they wanted to be on the other side of the date line, but now it's wrong as can be. Ooops. > But > debating individual points of geography and politics like this seems a > bit silly... I wasn't really aware of this Windows->Olson zone name > problem lurking in our tree before, but it sounds to me like switching > to 100% "we use CLDR, if you think it's wrong, please file a report at > cldr.unicode.org" wouldn't be a bad idea at all! I'd still defend our exception for Central America: CLDR maps that to Guatemala which seems pretty random, even if they haven't observed DST there for a few years. For the rest of it, though, "we follow CLDR" has definitely got a lot of attraction. The one change that makes me nervous is adopting Europe/Berlin for "W. Europe Standard Time", on account of the flak Paul Eggert just got from trying to make a somewhat-similar change :-(. (If you don't read the tz mailing list you may not be aware of that particular tempest in a teapot, but he tried to merge a bunch of zones into Europe/Berlin, and there were a lot of complaints. Some from me.) regards, tom lane
Re: Problem with CF app?
> When I click the mail archive link > (https://www.postgresql.org/message-id/flat/72a0d590d6ba06f242d75c2e64182...@postgrespro.ru) > in CF app web page of this entry: > https://commitfest.postgresql.org/34/3194/ > > I got: > > Error 503 Backend fetch failed > > Backend fetch failed > Guru Meditation: > > XID: 83477623 > > Varnish cache server > > Is there anything wrong with CF app? I can access the mail archive link now. It looks like a temporary failure. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: Timeout failure in 019_replslot_limit.pl
On 2021-Sep-27, Michael Paquier wrote: > I got again a failure today, so I have used this occasion to check that > when the checkpoint gets stuck the WAL sender process getting SIGCONT > is still around, waiting for a write to happen: > * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP > frame #0: 0x7fff20320c4a libsystem_kernel.dylib`kevent + 10 > frame #1: 0x00010fe50a43 > postgres`WaitEventSetWaitBlock(set=0x7f884d80a690, cur_timeout=-1, > occurred_events=0x7ffee0395fd0, nevents=1) at latch.c:1601:7 > frame #2: 0x00010fe4ffd0 > postgres`WaitEventSetWait(set=0x7f884d80a690, timeout=-1, > occurred_events=0x7ffee0395fd0, nevents=1, wait_event_info=100663297) at > latch.c:1396:8 > frame #3: 0x00010fc586c4 > postgres`secure_write(port=0x7f883eb04080, ptr=0x7f885006a040, > len=122694) at be-secure.c:298:3 > frame #4: 0x00010fc66d81 postgres`internal_flush at pqcomm.c:1352:7 > frame #5: 0x00010fc665b9 postgres`internal_putbytes(s="E, len=1) at > pqcomm.c:1298:8 > frame #6: 0x00010fc66be3 postgres`socket_putmessage(msgtype='E', > s="SFATAL", len=112) at pqcomm.c:1479:6 > frame #7: 0x00010fc67318 > postgres`pq_endmessage(buf=0x7ffee0396118) at pqformat.c:301:9 > frame #8: 0x0001100a469f > postgres`send_message_to_frontend(edata=0x00011030d640) at elog.c:3431:3 > frame #9: 0x0001100a066d postgres`EmitErrorReport at elog.c:1546:3 > frame #10: 0x00011009ff99 postgres`errfinish(filename="postgres.c", > lineno=3193, funcname="ProcessInterrupts") at elog.c:597:2 > * frame #11: 0x00010fe8e2f5 postgres`ProcessInterrupts at > postgres.c:3191:4 > frame #12: 0x00010fe0bbe5 > postgres`WalSndLoop(send_data=(postgres`XLogSendPhysical at > walsender.c:2550)) at walsender.c:2285:3 > frame #13: 0x00010fe0754f > postgres`StartReplication(cmd=0x7f881d808790) at walsender.c:738:3 > frame #14: 0x00010fe06149 > postgres`exec_replication_command(cmd_string="START_REPLICATION SLOT \"rep3\" > 0/70 TIMELINE 1") at walsender.c:1652:6 > frame #15: 0x00010fe91eb8 postgres`PostgresMain(dbname="", > username="mpaquier") at postgres.c:4493:12 Ah, so the problem here is that the walsender is not exiting. That also causes the checkpointer to hang waiting for it. I wonder if this is related to the problem reported in https://www.postgresql.org/message-id/adce2c09-3bfc-4666-997a-c21991cb1eb1.mengjuan.cmj%40alibaba-inc.com A patch was proposed on that thread on September 22nd, can to try with that and see if this problem still reproduces? -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "El sabio habla porque tiene algo que decir; el tonto, porque tiene que decir algo" (Platon).
Re: Adding CI to our tree
Andres Freund writes: > On 2021-10-02 16:44:44 -0400, Tom Lane wrote: >> If you'd like that, there would need to be some (ahem) documentation >> of how to use it. > Yea, definitely necessary. Where would we want it to be? ci/README.md? That'd > be viewable on the various git hosting platforms. I guess there's an argument > for it to be in the sgml docs, but that doesn't seem all that useful in this > case. A README seems plenty good enough to me. Maybe -0.1 for making it .md rather than plain text ... plain text is our habit everywhere else AFAIR. regards, tom lane
Re: 002_types.pl fails on some timezones on windows
On Sat, Oct 2, 2021 at 1:55 AM Tom Lane wrote: > BTW, I find those "territory" annotations in the CLDR data to be > fascinating. If that corresponds to something that we could retrieve > at runtime, it'd allow far better mapping of Windows zones than we > are doing now. I have no interest in working on that myself though. I wonder if it could be derived from the modern standards-based locale name, which we're not currently using as a default locale but probably should[1]. For single-zone countries you might be able to match exactly one zone mapping. [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com
Re: 002_types.pl fails on some timezones on windows
On Sun, Oct 3, 2021 at 9:42 AM Tom Lane wrote: > * Now-documented decision to map "Greenwich Standard Time" > to Europe/London, not Atlantic/Reykjavik as they have it. Hmm. It's hard to pick a default from that set of merged zones, but the funny thing about this choice is that Europe/London is the one Olson zone that it's sure *not* to be, because then your system would be using that other name, IIUC. > * The miscellaneous deltas shown in the attached diff, which in > many cases boil down to "we chose the first name mentioned for the > zone, while CLDR did something else". I felt that our historical > mappings of these cases weren't wrong enough to justify any > political flak I might take for changing them. OTOH, maybe we > should just say "we follow CLDR" and be done with it. Eyeballing these, three look strange to me in a list of otherwise city-based names: Pacific/Guam (instead of Port Moresby, capital of PNG which apparently shares zone rules with the territory of Guam) and Pacific/Samoa (country name instead of its capital Apia; the city avoids any potential confusion with American Samoa which is on the other side of the date line) and then "CET", an abbreviation. But debating individual points of geography and politics like this seems a bit silly... I wasn't really aware of this Windows->Olson zone name problem lurking in our tree before, but it sounds to me like switching to 100% "we use CLDR, if you think it's wrong, please file a report at cldr.unicode.org" wouldn't be a bad idea at all!
Re: Adding CI to our tree
Hi, On 2021-10-02 16:44:44 -0400, Tom Lane wrote: > Andrew Dunstan writes: > > I hope it will also encourage people to test more widely, given how easy > > it will make it. > > If you'd like that, there would need to be some (ahem) documentation > of how to use it. Yea, definitely necessary. Where would we want it to be? ci/README.md? That'd be viewable on the various git hosting platforms. I guess there's an argument for it to be in the sgml docs, but that doesn't seem all that useful in this case. Greetings, Andres Freund
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
Andrew Dunstan writes: > I haven't looked at the patch closely yet, but from a buildfarm POV I > think the only thing that needs to be done is to inhibit the buildfarm > client module if the TAP tests are present. The buildfarm code that runs > TAP tests should automatically detect and run the new test. > I've just counted and there are 116 animals reporting check-pg_upgrade, > so we'd better put that out pronto. It's a little early but I'll try to > push out a release containing code for it on Monday or Tuesday (it's a > one line addition). IIUC, the only problem for a non-updated animal would be that it'd run the test twice? Or would it actually fail? If the latter, we'd need to sit on the patch rather longer. regards, tom lane
Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set
On 10/1/21 2:19 AM, Michael Paquier wrote: > On Thu, May 20, 2021 at 03:07:56PM +0900, Michael Paquier wrote: >> This stuff still needs to be expanded depending on how PostgresNode is >> made backward-compatible, but I'll wait for that to happen before >> going further down here. I have also spent some time testing all that >> with MSVC, and the installation paths used for pg_regress&co make the >> script a tad more confusing, so I have dropped this part for now. > Andrew, as this is a bit tied to the buildfarm code and any > simplifications that could happen there, do you have any comments > and/or suggestions for this patch? I haven't looked at the patch closely yet, but from a buildfarm POV I think the only thing that needs to be done is to inhibit the buildfarm client module if the TAP tests are present. The buildfarm code that runs TAP tests should automatically detect and run the new test. I've just counted and there are 116 animals reporting check-pg_upgrade, so we'd better put that out pronto. It's a little early but I'll try to push out a release containing code for it on Monday or Tuesday (it's a one line addition). cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Adding CI to our tree
Andrew Dunstan writes: > I hope it will also encourage people to test more widely, given how easy > it will make it. If you'd like that, there would need to be some (ahem) documentation of how to use it. regards, tom lane
Re: 002_types.pl fails on some timezones on windows
I wrote:
> Oh, thanks for the pointer to CLDR! I tried re-generating our data
> based on theirs, and ended up with the attached draft patch.
Hearing no objections, pushed after another round of review
and a couple more fixes.
For the archives' sake, here are the remaining discrepancies
between our mapping and CLDR's entries for "territory 001",
which I take to be their recommended defaults:
* Our documented decision to map "Central America" to "CST6",
on the grounds that most of Central America doesn't actually
observe DST nowadays.
* Now-documented decision to map "Greenwich Standard Time"
to Europe/London, not Atlantic/Reykjavik as they have it.
* The miscellaneous deltas shown in the attached diff, which in
many cases boil down to "we chose the first name mentioned for the
zone, while CLDR did something else". I felt that our historical
mappings of these cases weren't wrong enough to justify any
political flak I might take for changing them. OTOH, maybe we
should just say "we follow CLDR" and be done with it.
regards, tom lane
--- findtimezone.c 2021-10-02 15:37:17.309929827 -0400
+++ cldr_transformed.c 2021-10-02 14:32:32.359818338 -0400
@@ -888,7 +131,7 @@ static const struct
{
/* (UTC+01:00) Belgrade, Bratislava, Budapest, Ljubljana, Prague */
"Central Europe Standard Time", "Central Europe Daylight Time",
- "Europe/Belgrade"
+ "Europe/Budapest"
},
{
/* (UTC+01:00) Sarajevo, Skopje, Warsaw, Zagreb */
@@ -898,7 +141,7 @@ static const struct
{
/* (UTC+11:00) Solomon Is., New Caledonia */
"Central Pacific Standard Time", "Central Pacific Daylight Time",
- "Pacific/Noumea"
+ "Pacific/Guadalcanal"
},
{
/* (UTC-06:00) Central Time (US & Canada) */
@@ -988,7 +226,7 @@ static const struct
{
/* (UTC+02:00) Helsinki, Kyiv, Riga, Sofia, Tallinn, Vilnius */
"FLE Standard Time", "FLE Daylight Time",
- "Europe/Helsinki"
+ "Europe/Kiev"
},
{
/* (UTC+04:00) Tbilisi */
@@ -1006,7 +244,7 @@ static const struct
{
/* (UTC+02:00) Athens, Bucharest */
"GTB Standard Time", "GTB Daylight Time",
- "Europe/Athens"
+ "Europe/Bucharest"
},
{
/* (UTC-05:00) Haiti */
@@ -1244,7 +441,7 @@ static const struct
{
/* (UTC+01:00) Brussels, Copenhagen, Madrid, Paris */
"Romance Standard Time", "Romance Daylight Time",
- "Europe/Brussels"
+ "Europe/Paris"
},
{
/* (UTC+11:00) Chokurdakh */
@@ -1349,7 +491,7 @@ static const struct
{
/* (UTC+13:00) Samoa */
"Samoa Standard Time", "Samoa Daylight Time",
- "Pacific/Samoa"
+ "Pacific/Apia"
},
{
/* (UTC+00:00) Sao Tome */
@@ -1519,7 +661,7 @@ static const struct
{
/* (UTC+01:00) Amsterdam, Berlin, Bern, Rome, Stockholm, Vienna */
"W. Europe Standard Time", "W. Europe Daylight Time",
- "CET"
+ "Europe/Berlin"
},
{
/* (UTC+07:00) Hovd */
@@ -1533,7 +675,7 @@ static const struct
{
/* (UTC+10:00) Guam, Port Moresby */
"West Pacific Standard Time", "West Pacific Daylight Time",
- "Pacific/Guam"
+ "Pacific/Port_Moresby"
},
{
/* (UTC+09:00) Yakutsk */
Re: Adding CI to our tree
On 10/2/21 11:05 AM, Tom Lane wrote: > Andres Freund writes: >> It's not like this forces you to use cirrus or anything. For people that >> don't >> want to use CI, It'll make cfbot a bit more effective (because people can >> adjust what it tests as appropriate for $patch), but that's it. > Yeah. I cannot see any reason to object to Andres' 0002 patch: you can > just ignore those files if you don't want to use cirrus. Yeah. I enable cirrus selectively on my github repos, which makes it close to impossible to get an unwanted effect. One of the things I like about this is that it institutionalizes some knowledge that has hitherto been mostly private. I have a lot of this in a setup I use for spinning up test instances, but this makes a lot of that sort of knowledge more broadly available. I hope it will also encourage people to test more widely, given how easy it will make it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Adding CI to our tree
> On 2 Oct 2021, at 22:01, Andres Freund wrote:
> On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:
>> Same, and for my case I run several CI jobs to compile/test against different
>> OpenSSL versions etc.
>
> On that note: Did you do this for windows? If so, I'd rather not figure that
> out myself...
Not with Cirrus, I've been using Appveyor for Windows and they provide 1.0.2 -
3.0.0 which can easily set in config.pl with for example:
$config->{openssl} = 'C:\OpenSSL-v111-Win64';
--
Daniel Gustafsson https://vmware.com/
Re: Adding CI to our tree
Hi, On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote: > Same, and for my case I run several CI jobs to compile/test against different > OpenSSL versions etc. On that note: Did you do this for windows? If so, I'd rather not figure that out myself... Greetings, Andres Freund
Re: Adding CI to our tree
Hi, On 2021-10-02 11:05:20 -0400, Tom Lane wrote: > Yeah. I cannot see any reason to object to Andres' 0002 patch: you can > just ignore those files if you don't want to use cirrus. It does set a > precedent that we'd also accept infrastructure for other CI systems, > but as long as they're similarly noninvasive, why not? Exactly. > (Maybe there needs to be one more directory level though, ie > ci/cirrus/whatever. I don't want to end up with one toplevel directory per > CI platform.) Good question - it definitely shouldn't be one toplevel directory per CI platform (although some will require their own hidden toplevel directories, like .github/workflows etc). I'd hope to share a bunch of the infrastructure between them over time, so perhaps we don't need a deeper hierarchy. > I don't know enough about Windows to evaluate 0001, but I'm a little > worried about it because it looks like it's changing our *production* > error handling on that platform. Yea. It's clearly not ready as-is - it's the piece that I was planning to write a separate email about. It's hard to understand what *precisely* SEM_NOGPFAULTERRORBOX etc do. What I do know is that without the _set_abort_behavior() stuff abort() doesn't trigger windows' "crash" paths in at least debugging builds, and that the SetErrorMode() and _CrtSetReportMode() changes are necessary to get segfaults to reach the crash paths. The in-tree behaviour turns out to make debugging on windows a major pain, at least when compiling with msvc. Crashes never trigger core dumps or "just in time" debugging (their term for invoking a debugger upon crash), so one has to attach to processes before they crash, to have any chance of debugging. As far as I can tell this also means that at least for debugging builds, pgwin32_install_crashdump_handler() is pretty much dead weight - crashDumpHandler() never gets invoked. I think it may get invoked for abort()s in production builds, but probably not for segfaults. And despite SEM_NOGPFAULTERRORBOX we display those annoying "popup" boxes telling us about the crash and giving the option to retry, ignore, something something. It's all a bit baffling. > As for 0003, wasn't that committed already? Not at the time I was writing the email, but now it is, yes. Greetings, Andres Freund
Re: Adding CI to our tree
> On 2 Oct 2021, at 21:41, Andres Freund wrote:
>> One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
>> that it intermittently will fail on a too long socketpath:
>
> I've seen it somewhere else before. It wasn't even intermittent - it always
> failed. I worked around that by setting CIRRUS_WORKING_DIR: ${HOME}/pgsql/ -
> also made output including filenames easier to read ;)
Aha, nice trick! Didn't know about that one but that's easier than setting
specific dirs via PG* environment vars.
--
Daniel Gustafsson https://vmware.com/
Re: Adding CI to our tree
> On 2 Oct 2021, at 21:45, Andres Freund wrote:
> I wonder if we shouldn't stop skipping the ssl / kerberos / ldap (and perhaps
> others) tests in the makefile, and instead do so in the tap tests
> themselves. Then one can see them included as the skipped in the tap result
> output, which seems like it'd make it easier to discover them?
I am definitely in favor of doing that, better to see them skipped rather than
having to remember to opt in. We even do so already to some extent already,
like for example the SSL tests:
if ($ENV{with_ssl} ne 'openssl')
{
plan skip_all => 'OpenSSL not supported by this build';
}
--
Daniel Gustafsson https://vmware.com/
Re: Adding CI to our tree
Hi,
On 2021-10-02 12:41:07 -0700, Andres Freund wrote:
> On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:
> > + tests_script:
> > +- su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s
> > ${CHECK} ${CHECKFLAGS} -j8'
> > Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are
> > run?
>
> Probably. I quickly added that stuff, we'll see how many mistakes I made:
> https://cirrus-ci.com/build/5846034501861376
I wonder if we shouldn't stop skipping the ssl / kerberos / ldap (and perhaps
others) tests in the makefile, and instead do so in the tap tests
themselves. Then one can see them included as the skipped in the tap result
output, which seems like it'd make it easier to discover them?
Greetings,
Andres Freund
Re: Adding CI to our tree
Hi,
On 2021-10-02 20:42:00 +0200, Daniel Gustafsson wrote:
> > On 2 Oct 2021, at 00:27, Andres Freund wrote:
> > Right now the patch attached
> > - runs check-world on FreeBSD, Linux, macOS - all using gcc
> > - freebsd, linux use a custom generated image
> > - macOS installs missing dependencies at runtime, with some caching
> > - all use ccache to make subsequent compilation faster
> > - runs all the tests I could find on windows, via vcregress.pl
> > - checks for compiler warnings on linux, with both clang and gcc
>
> Why not compiling with OpenSSL on FreeBSD and macOS? On FreeBSD all you need
> is --with-ssl=openssl while on macOS you need to point to the headers and libs
> like:
>
> --with-includes=/usr/local/include:/usr/local/opt/openssl/include
> --with-libs=/usr/local/libs:/usr/local/opt/openssl/lib
Yea, there's several things like that, that should be added. The CI files
originated from development where breakage around SSL wasn't likely (AIO,
shared memory stats, procarray scalability etc), so I didn't focussed on that
angle.
Needing to get all that stuff right on multiple platforms is one of the
reasons why I think having this thing in-tree would be good. No need for
everyone to discover the magic incantations themselves. Even if you e.g. might
want to extend them to test multiple SSL versions or such, it's a lot easier
to do that if the basics are there.
> One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
> that it intermittently will fail on a too long socketpath:
I've seen it somewhere else before. It wasn't even intermittent - it always
failed. I worked around that by setting CIRRUS_WORKING_DIR: ${HOME}/pgsql/ -
also made output including filenames easier to read ;)
> + tests_script:
> +- su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s ${CHECK}
> ${CHECKFLAGS} -j8'
> Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are
> run?
Probably. I quickly added that stuff, we'll see how many mistakes I made:
https://cirrus-ci.com/build/5846034501861376
Greetings,
Andres Freund
Re: Adding CI to our tree
> On 2 Oct 2021, at 00:27, Andres Freund wrote:
> For several development efforts I found it to be incredibly valuable to push
> changes to a personal repository and see a while later whether tests succeed
> on a number of different platforms. This is especially useful for platforms
> that are quite different from ones own platform, like e.g. windows in my case.
Same, and for my case I run several CI jobs to compile/test against different
OpenSSL versions etc.
> Of course everybody can set this up for themselves. However, doing so well is
> a significant effort, particularly if windows is to be supported well. And
> doubly so if useful things like getting backtraces for crashes is desirable
> ([1])
+1 on adding these, rather than having everyone duplicate the effort. Those
who don't want to use them can disregard them.
> Right now the patch attached
> - runs check-world on FreeBSD, Linux, macOS - all using gcc
> - freebsd, linux use a custom generated image
> - macOS installs missing dependencies at runtime, with some caching
> - all use ccache to make subsequent compilation faster
> - runs all the tests I could find on windows, via vcregress.pl
> - checks for compiler warnings on linux, with both clang and gcc
Why not compiling with OpenSSL on FreeBSD and macOS? On FreeBSD all you need
is --with-ssl=openssl while on macOS you need to point to the headers and libs
like:
--with-includes=/usr/local/include:/usr/local/opt/openssl/include
--with-libs=/usr/local/libs:/usr/local/opt/openssl/lib
One thing to note for Cirrus on macOS (I've never seen it anywhere else) is
that it intermittently will fail on a too long socketpath:
Unix-domain socket path
"/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1mgn/T/cirrus-ci-build/src/bin/pg_upgrade/.s.PGSQL.51696"
is too long (maximum 103 bytes)
Exporting PGSOCKETDIR can avoid that annoyance.
+ tests_script:
+- su postgres -c 'ulimit -c unlimited ; ${TIMEOUT_CMD} make -s ${CHECK}
${CHECKFLAGS} -j8'
Don't you need PG_TEST_EXTRA=ssl here to ensure the src/test/ssl tests are run?
--
Daniel Gustafsson https://vmware.com/
Re: 2021-09 Commitfest
> On 2 Oct 2021, at 17:24, Tom Lane wrote: > > Alvaro Herrera writes: >> I think if we all agree that this is a desired workflow, then we should >> update the app to allow WoA patches to be moved to next CF. > > I'm fairly astonished that anyone would have thought that that > *wasn't* an expected case. AFAIK this is a case of everyone agreeing and noone having had the time (or priorities) to hack on the CF app to make it happen. -- Daniel Gustafsson https://vmware.com/
Re: Better context for "TAP tests not enabled" error message
Daniel Gustafsson writes: >> On 2 Oct 2021, at 02:09, Kevin Burke wrote: >> This patch amends the error message to give help to the user. > I think this makes sense, +1. I'd take out the "Maybe"; the diagnosis seems pretty certain. regards, tom lane
Re: Better context for "TAP tests not enabled" error message
> On 2 Oct 2021, at 02:09, Kevin Burke wrote: > This patch amends the error message to give help to the user. I think this makes sense, and is in line with Rachels patch [0] a few days ago that I plan on pushing; small error hints which wont get in the way of established developers, but which can help new developers onboard onto our tree and processes around it. -- Daniel Gustafsson https://vmware.com/ [0] https://postgr.es/m/cadjcwivl20955hcnzdqz9bedr6a77pz6-nac5sbzvvhaemi...@mail.gmail.com
Re: [PATCH] Error out if SKIP LOCKED and WITH TIES are both specified
On Fri, Oct 01, 2021 at 06:33:01PM -0300, Alvaro Herrera wrote: > On 2021-Aug-13, David Christensen wrote: > > > Both bugs #16676[1] and #17141[2] illustrate that the combination of > > SKIP LOCKED and FETCH FIRST WITH TIES break expectations when it comes > > to rows returned to other sessions accessing the same row. Since this > > situation is detectable from the syntax and hard to fix otherwise, > > forbid for now, with the potential to fix in the future. > > Thank you, pushed with minimal adjustment. > BTW, I just marked this one as committed in CF app -- Jaime Casanova Director de Servicios Profesionales SystemGuards - Consultores de PostgreSQL
Better context for "TAP tests not enabled" error message
I got the error message in the subject and was unsure how to continue; I didn't see any hits for the error message on the mailing list, and it was hard to determine from the context around the error in the Makefile.global.in about the best way to solve the problem. This patch amends the error message to give help to the user. I ran "make check" at the top level with this patch enabled and 209 tests passed. I also ran "make check" in src/test/ssl without TAP enabled and verified that I got the new error message. I also verified that compiling with --enable-tap-tests fixes the error in question. This patch does not include regression tests. Another way to fix this issue could be to put the exact text of the error message in the documentation or the wiki, with instructions on how to fix it - the first thing I did was punch the error message into Google, if a match for the error message came up with instructions on how to fix it, that would also help. This is the first patch that I've submitted to Postgres, I believe that I've followed the guidelines on the patch submission page, but please let me know if I missed anything. Kevin -- Kevin Burke phone: 925-271-7005 | kevin.burke.dev 0001-Makefile-provide-better-help-if-TAP-tests-are-not-en.patch Description: Binary data
Re: should frontend tools use syncfs() ?
On Thu, Sep 30, 2021 at 05:08:24PM +1300, Thomas Munro wrote: > On Thu, Sep 30, 2021 at 4:49 PM Michael Paquier wrote: > > fsync_pgdata() is going to manipulate many inodes anyway, because > > that's a code path designed to do so. If we know that syncfs() is > > just going to be better, I'd rather just call it by default if > > available and not add new switches to all the frontend tools in need > > of flushing the data folder, switches that are not documented in your > > patch. > > If we want this it should be an option, because it flushes out data > other than the pgdata dir, and it doesn't report errors on old > kernels. I ran into bad performance of initdb --sync-only shortly after adding it to my db migration script, so added initdb --syncfs. I found that with sufficiently recent coreutils, I can do what's wanted by calling /bin/sync -f /datadir Since it's not integrated into initdb, it's necessary to include each tablespace and wal. -- Justin
Re: 2021-09 Commitfest
Alvaro Herrera writes: > On 2021-Oct-02, Tom Lane wrote: >> Yeah. I have been thinking of looking through the oldest CF entries >> and proposing that we just reject any that look permanently stalled. > I was just going to say the same thing yesterday, and reference [1] > when I did it once in 2019. I think it was a useful cleanup exercise. > [1] https://postgr.es/m/20190930182818.GA25331@alvherre.pgsql Right. Michael and Jaime have been doing some of that too in the last few days, but obviously a CFM should only do that unilaterally in very clear-cut cases of patch abandonment. I was intending to go after some where maybe a bit of community consensus is needed for rejection. regards, tom lane
Re: 2021-09 Commitfest
Alvaro Herrera writes: > I think if we all agree that this is a desired workflow, then we should > update the app to allow WoA patches to be moved to next CF. I'm fairly astonished that anyone would have thought that that *wasn't* an expected case. For example, if someone reviews a patch and sets the status to WoA on the last day of the CF, what then? You can't expect the patch author to respond instantaneously. regards, tom lane
Re: 2021-09 Commitfest
On 2021-Oct-02, Tom Lane wrote: > Yeah. I have been thinking of looking through the oldest CF entries > and proposing that we just reject any that look permanently stalled. > It doesn't do much good to leave things in the list when there's > no apparent interest in pushing them to conclusion. But I've not > done the legwork yet, and I'm a little worried about the push-back > that will inevitably result. I was just going to say the same thing yesterday, and reference [1] when I did it once in 2019. I think it was a useful cleanup exercise. In hindsight, some of these patches were resubmitted later, and those are either still ongoing or are already committed. [1] https://postgr.es/m/20190930182818.GA25331@alvherre.pgsql (I did have the luxury of a local copy of the commitfest database, which is perhaps a service we could offer to CFMs to make their lives easier.) -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Digital and video cameras have this adjustment and film cameras don't for the same reason dogs and cats lick themselves: because they can." (Ken Rockwell)
Re: 2021-09 Commitfest
On 2021-Oct-01, Daniel Gustafsson wrote: > > On 1 Oct 2021, at 19:49, Jaime Casanova > > wrote: > > I will do that but we should improve that process. > > Correct again. I think if we all agree that this is a desired workflow, then we should update the app to allow WoA patches to be moved to next CF. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La fuerza no está en los medios físicos sino que reside en una voluntad indomable" (Gandhi)
Re: Adding CI to our tree
Andres Freund writes: > It's not like this forces you to use cirrus or anything. For people that don't > want to use CI, It'll make cfbot a bit more effective (because people can > adjust what it tests as appropriate for $patch), but that's it. Yeah. I cannot see any reason to object to Andres' 0002 patch: you can just ignore those files if you don't want to use cirrus. It does set a precedent that we'd also accept infrastructure for other CI systems, but as long as they're similarly noninvasive, why not? (Maybe there needs to be one more directory level though, ie ci/cirrus/whatever. I don't want to end up with one toplevel directory per CI platform.) I don't know enough about Windows to evaluate 0001, but I'm a little worried about it because it looks like it's changing our *production* error handling on that platform. As for 0003, wasn't that committed already? regards, tom lane
Re: 2021-09 Commitfest
Michael Paquier writes: > That's the tricky part. It does not really make sense either to keep > moving patches that are waiting on author for months. The scan of the > CF app I have done was about those idle patches waiting on author for > months. It takes time as authors and/or reviewers tend to sometimes > not update the status of a patch so the state in the app does not > reflect the reality, but this vacuuming limits the noise in for the > next CFs. Yeah. I have been thinking of looking through the oldest CF entries and proposing that we just reject any that look permanently stalled. It doesn't do much good to leave things in the list when there's no apparent interest in pushing them to conclusion. But I've not done the legwork yet, and I'm a little worried about the push-back that will inevitably result. regards, tom lane
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On 2021-Oct-02, Dilip Kumar wrote: > I have written two patches, Approach1 is as you described using a > static boolean and Approach2 as a local variable to XLogAssembleRecord > as described by Amit, attached both of them for your reference. > IMHO, either of these approaches looks cleaner. Thanks! I haven't read these patches carefully, but I think the variable is about assigning the *subxid*, not the topxid. Amit can confirm ... -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)
Re: psql - add SHOW_ALL_RESULTS option
Hello Peter,
Attached v9 integrates your tests and makes them work.
Attached v11 is a rebase.
--
Fabien.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index b52d187722..0cf4a37a5f 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -50,8 +50,28 @@ BEGIN \;
SELECT 2.0 AS "float" \;
SELECT 'world' AS "text" \;
COMMIT;
+ float
+---
+ 2.0
+(1 row)
+
+ text
+---
+ world
+(1 row)
+
-- compound with empty statements and spurious leading spacing
\;\; SELECT 3 + 3 \;\;\; SELECT ' ' || ' !' \;\; SELECT 1 + 4 \;;
+ ?column?
+--
+6
+(1 row)
+
+ ?column?
+--
+ !
+(1 row)
+
?column?
--
5
@@ -61,6 +81,11 @@ COMMIT;
SELECT 1 + 1 + 1 AS "add" \gset
SELECT :add + 1 + 1 AS "add" \;
SELECT :add + 1 + 1 AS "add" \gset
+ add
+-
+ 5
+(1 row)
+
-- set operator
SELECT 1 AS i UNION SELECT 2 ORDER BY i;
i
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 14e0a4dbe3..6866a06f2d 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -127,18 +127,11 @@ echo '\x \\ SELECT * FROM foo;' | psql
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
- Also, psql only prints the
- result of the last SQL command in the string.
- This is different from the behavior when the same string is read from
- a file or fed to psql's standard input,
- because then psql sends
- each SQL command separately.
- Because of this behavior, putting more than one SQL command in a
- single -c string often has unexpected results.
- It's better to use repeated -c commands or feed
- multiple commands to psql's standard input,
+ If having several commands executed in one transaction is not desired,
+ use repeated -c commands or feed multiple commands to
+ psql's standard input,
either using echo as illustrated above, or
via a shell here-document, for example:
@@ -3542,10 +3535,6 @@ select 1\; select 2\; select 3;
commands included in the string to divide it into multiple
transactions. (See
for more details about how the server handles multi-query strings.)
-psql prints only the last query result
-it receives for each request; in this example, although all
-three SELECTs are indeed executed, psql
-only prints the 3.
@@ -4132,6 +4121,18 @@ bar
+SHOW_ALL_RESULTS
+
+
+When this variable is set to off, only the last
+result of a combined query (\;) is shown instead of
+all of them. The default is on. The off behavior
+is for compatibility with older versions of psql.
+
+
+
+
+
SHOW_CONTEXT
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 5640786678..481a316fed 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -33,6 +33,8 @@ static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
static bool is_select_command(const char *query);
+static int SendQueryAndProcessResults(const char *query, double *pelapsed_msec,
+ bool is_watch, const printQueryOpt *opt, FILE *printQueryFout, bool *tx_ended);
/*
@@ -353,7 +355,7 @@ CheckConnection(void)
* Returns true for valid result, false for error state.
*/
static bool
-AcceptResult(const PGresult *result)
+AcceptResult(const PGresult *result, bool show_error)
{
bool OK;
@@ -384,7 +386,7 @@ AcceptResult(const PGresult *result)
break;
}
- if (!OK)
+ if (!OK && show_error)
{
const char *error = PQerrorMessage(pset.db);
@@ -472,6 +474,18 @@ ClearOrSaveResult(PGresult *result)
}
}
+/*
+ * Consume all results
+ */
+static void
+ClearOrSaveAllResults()
+{
+ PGresult *result;
+
+ while ((result = PQgetResult(pset.db)) != NULL)
+ ClearOrSaveResult(result);
+}
+
/*
* Print microtiming output. Always print raw milliseconds; if the interval
@@ -572,7 +586,7 @@ PSQLexec(const char *query)
ResetCancelConn();
- if (!AcceptResult(res))
+ if (!AcceptResult(res, true))
{
ClearOrSaveResult(res);
res = NULL;
@@ -594,11 +608,8 @@ PSQLexec(const char *query)
int
PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
- PGresult *res;
double elapsed_msec = 0;
- instr_time before;
- instr_time after;
- FILE *fout;
+ int res;
if (!pset.db)
{
@@ -607,77 +618,14 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt, FI
Re: Slow standby snapshot
Hello, Andres. Could you please clarify how to better deal with the situation? According to your previous letter, I think there was some misunderstanding regarding the latest patch version (but I am not sure). Because as far as understand provided optimization (lazily calculated optional offset to the next valid xid) fits into your wishes. It was described in the previous letter in more detail. And now it is not clear for me how to move forward :) There is an option to try to find some better data structure (like some tricky linked hash map) but it is going to be huge changes without any confidence to get a more effective version (because provided changes make the structure pretty effective). Another option I see - use optimization from the latest patch version and get a significant TPS increase (5-7%) for the typical standby read scenario. Patch is small and does not affect other scenarios in a negative way. Probably I could make an additional set of some performance tests and provide some simulation to prove that pg_atomic_uint32-related code is correct (if required). Or just leave the issue and hope someone else will try to fix it in the future :) Thanks a lot, Michail.
Re: pgcrypto support for bcrypt $2b$ hashes
On Sat, 2 Oct 2021 at 23:22, Andreas Karlsson wrote: > Please add your patch to the current open commitfest. > Done. https://commitfest.postgresql.org/35/3338/ Thanks for the guidance. Daniel
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera wrote:
>
> On 2021-Oct-01, Amit Kapila wrote:
> I think a straight standalone variable (probably a static boolean in
> xloginsert.c) might be less confusing.
I have written two patches, Approach1 is as you described using a
static boolean and Approach2 as a local variable to XLogAssembleRecord
as described by Amit, attached both of them for your reference.
IMHO, either of these approaches looks cleaner.
>
> ... so, reading the xact.c code again, TransactionState->assigned really
> means "whether the subXID-to-topXID association has been wal-logged",
> which is a completely different meaning from what the term 'assigned'
> means in all other comments in xact.c ... and I think the subroutine
> name MarkSubTransactionAssigned() is not a great choice either.
I have also renamed the variable and functions as per the actual usage.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
From 3d4fb94fdeef2ae46511291f144b6305c9eea9f7 Mon Sep 17 00:00:00 2001
From: Dilip Kumar
Date: Sat, 2 Oct 2021 15:21:52 +0530
Subject: [PATCH] Refactor code for logging the top transaction id in WAL
---
src/backend/access/transam/xact.c | 22 +++---
src/backend/access/transam/xlog.c | 7 ++-
src/backend/access/transam/xloginsert.c | 23 ++-
src/include/access/xact.h | 4 ++--
src/include/access/xlog.h | 4 ++--
5 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4cc38f0..893147649 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -204,7 +204,7 @@ typedef struct TransactionStateData
bool didLogXid; /* has xid been included in WAL record? */
int parallelModeLevel; /* Enter/ExitParallelMode counter */
bool chain; /* start a new block after this one */
- bool assigned; /* assigned to top-level XID */
+ bool topXidLogged; /* top-level XID is logged?*/
struct TransactionStateData *parent; /* back link to parent */
} TransactionStateData;
@@ -237,7 +237,7 @@ typedef struct SerializedTransactionState
static TransactionStateData TopTransactionStateData = {
.state = TRANS_DEFAULT,
.blockState = TBLOCK_DEFAULT,
- .assigned = false,
+ .topXidLogged = false,
};
/*
@@ -5159,7 +5159,7 @@ PushTransaction(void)
GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
s->prevXactReadOnly = XactReadOnly;
s->parallelModeLevel = 0;
- s->assigned = false;
+ s->topXidLogged = false;
CurrentTransactionState = s;
@@ -6093,7 +6093,7 @@ xact_redo(XLogReaderState *record)
}
/*
- * IsSubTransactionAssignmentPending
+ * IsLogTopTransactionIdPending
*
* This is used to decide whether we need to WAL log the top-level XID for
* operation in a subtransaction. We require that for logical decoding, see
@@ -6104,7 +6104,7 @@ xact_redo(XLogReaderState *record)
* record.
*/
bool
-IsSubTransactionAssignmentPending(void)
+IsLogTopTransactionIdPending(void)
{
/* wal_level has to be logical */
if (!XLogLogicalInfoActive())
@@ -6123,18 +6123,18 @@ IsSubTransactionAssignmentPending(void)
return false;
/* and it should not be already 'assigned' */
- return !CurrentTransactionState->assigned;
+ return !CurrentTransactionState->topXidLogged;
}
/*
- * MarkSubTransactionAssigned
+ * MarkTopTransactionIdLogged
*
- * Mark the subtransaction assignment as completed.
+ * Mark top transaction id is WAL logged for the current subtransaction.
*/
void
-MarkSubTransactionAssigned(void)
+MarkTopTransactionIdLogged(void)
{
- Assert(IsSubTransactionAssignmentPending());
+ Assert(IsLogTopTransactionIdPending());
- CurrentTransactionState->assigned = true;
+ CurrentTransactionState->topXidLogged = true;
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f8c714b..78232aa 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1010,7 +1010,8 @@ XLogRecPtr
XLogInsertRecord(XLogRecData *rdata,
XLogRecPtr fpw_lsn,
uint8 flags,
- int num_fpi)
+ int num_fpi,
+ bool topxid_included)
{
XLogCtlInsert *Insert = &XLogCtl->Insert;
pg_crc32c rdata_crc;
@@ -1163,6 +1164,10 @@ XLogInsertRecord(XLogRecData *rdata,
MarkCurrentTransactionIdLoggedIfAny();
+ /* Mark top transaction id is logged (if needed) */
+ if (topxid_included)
+ MarkTopTransactionIdLogged();
+
END_CRIT_SECTION();
/*
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index b492c65..6b5925e 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -123,7 +123,8 @@ static MemoryContext xloginsert_cxt;
static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr RedoRecPtr, bool doPageWrites,
- XLogRecPtr *fpw_lsn, int *num_fpi);
+ XLogRe
Re: pgcrypto support for bcrypt $2b$ hashes
On 10/2/21 5:48 AM, Daniel Fone wrote: I don’t get these compiler warnings and I can’t find any settings to use that might generate them. I’m compiling on macOS 11.6 configured with `--enable-cassert --enable-depend --enable-debug CFLAGS=-O0` I’ve optimistically updated the patch to hopefully address them, but I’d like to know what I need to do to get those warnings. I run "gcc (Debian 10.3.0-11) 10.3.0" and your new patch silenced the warnings. Please add your patch to the current open commitfest. https://commitfest.postgresql.org/35/ Andreas
Re: Added schema level support for publication.
On Thu, Sep 30, 2021 at 3:39 PM vignesh C wrote: > > The suggested change works, I have modified it in the attached patch. > I have reviewed the latest version and made a number of changes to the 0001 patch. The changes are in v1-0001-Changes-by-Amit. It includes (a) Changed preprocess_pubobj_list() to make the code easy to understand, (b) the handling of few variables was missing in equal function, (c) the ordering of functions, and a few parameters were not matching .c and .h files, (d) added/edited multiple comments and other cosmetic changes. Apart from that, I have few other comments: 1. It seems you have started using unique list variants in GetPubPartitionOptionRelations because one of its caller GetSchemaPublicationRelations need it. I think the unique variants are costlier, so isn't it better to use it where it is required? I think it would be good to use in GetPubPartitionOptionRelations, if most of its caller requires the same. 2. In GetSchemaPublicationRelations(), I think we need to perform a second scan using RELKIND_PARTITIONED_TABLE only if we publish_via_root (aka pub_partopt is PUBLICATION_PART_ROOT). This is what we are doing in GetAllTablesPublicationRelations. Is there a reason to be different here? 3. @@ -538,7 +788,7 @@ RemovePublicationById(Oid pubid) if (!HeapTupleIsValid(tup)) elog(ERROR, "cache lookup failed for publication %u", pubid); - pubform = (Form_pg_publication)GETSTRUCT(tup); + pubform = (Form_pg_publication) GETSTRUCT(tup); We don't need the above change for this patch. I think this may be due pgindent but we can do this separately rather than as part of this patch. -- With Regards, Amit Kapila. v35-0001-Added-schema-level-support-for-publication.patch Description: Binary data v1-0001-Changes-by-Amit.patch Description: Binary data
