Hello hackers,
One of the area that didn't get much attention in the community
recently is analysing and increasing the code coverage of
PostgreSQL regession test suite. I have started working on the
code coverage by running the GCOV code coverage analysis tool in
order to analyse the current
>Hi Movead,
>Please add that to commitfest.
Thanks and I just do it, it is
https://commitfest.postgresql.org/24/2258/
--
Movead Li
On Tue, 27 Aug 2019 14:07:48 +0800 mich...@paquier.xyz wrote:
> There is a section in float4.sql which deals with overflow and
> underflow, so wouldn't it be better to move the tests there? You
> could just trigger the failures with that:
> =# insert into float4_tbl values ('-10e-70'::float8);
On Wed, 28 Aug 2019 11:30:23 +0800 mich...@paquier.xyz wrote
>- numeric is not a test suite designed for sorting, and hijacking it
>to do so it not a good approach.
Absolutely agreement. We can wait for repling from
Robert and Peter G.
>- it would be good to get coverage for the two extra code
On Mon, 26 Aug 2019 12:48:40 +0800 mich...@paquier.xyz wrote
>Your patch has forgotten to update the alternate output in
>float4-misrounded-input.out.
Thanks for your remind, I have modified the patch and now it is
'regression_20190826.patch' in attachment, and I have done a successful
test on
On 2019-08-29 00:43, peter.eisentr...@2ndquadrant.com wrote:
> Make spaces and capitalization match surrounding code.
>That's fine, but I suggest that if you really want to make an impact in
>test coverage, look to increase function coverage rather than line
>coverage. Or look for files that
>Which regression?
Apply the 0001.patch~0005.patch and then do a 'make check', then there be a
failed item. And when you apply the 0006.patch, the failed item disappeared.
>There should be an error because you don't have a PK or REPLICA IDENTITY.
No. I have done the 'ALTER TABLE cities REPLICA
Hello,
>It is "asynchronous append on async-capable'd postgres-fdw scans". It
>could be called as such in the sense that it is intended to be used
>with sharding.
Yes that's it.
>Did you looked at my benchmarking result upthread? Even it gives
>significant gain even when gathering large
Hello
I have tested the patch with a partition table with several foreign
partitions living on seperate data nodes. The initial testing was done
with a partition table having 3 foreign partitions, test was done with
variety of scale facters. The seonnd test was with fixed data per data
node but
>It seems to me that you are setting a path containing ".." to PGDATA.
Thanks point it for me.
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Hello hackers,
I found an issue about get_bit() and set_bit() function,here it is:
postgres=# select
get_bit(pg_read_binary_file('/home/movead/temp/file_seek/f_512M'), 0);
2020-03-12 10:05:23.296 CST [10549] ERROR: index 0 out of valid range, 0..-1
2020-03-12
Thanks for the reply.
>Why have you used size? Shouldn't we use int64?
Yes, thanks for the point, I have changed the patch.
>If get_bit()/set_bit() accept the second argument as int32, it can not
>be used to set bits whose number does not fit 32 bits. I think we need
>to change the type of the
>> we should notice it in document I think.
>There is the following explation about the relationship the recovery
>pause and the promotion, in the document. You may want to add more
>descriptions into the docs?
>--
>If a promotion is triggered while recovery is
>In existing releases, the SQL definitions are set_bit(bytea,int4,int4)
>and get_bit(bytea,int4) and cannot be changed to not break the API.
>So the patch meant for existing releases has to deal with a too-narrow
>int32 bit number.
>Internally in the C functions, you may convert that number to
Hello hackers,
After several patch change by hacker's proposal, I think it's ready to
commit, can we commit it before doing the code freeze for pg-13?
Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
>To cover your case, what about adding the following description?
>-
>There can be delay between a promotion request by users and the trigger of
>a promotion in the server. Note that pg_wal_replay_pause() succeeds
>during that delay, i.e., until a promotion is actually
>+ int64 res,resultlen;
>It's better to have them on separate lines.
Sorry for that, done.
>-unsigned
>+int64
> hex_decode(const char *src, unsigned len, char *dst)
>Do we want to explicitly cast the return value to int64? Will build on some
>platform crib if not done so?
>I don't know of such
>Thanks for the explanation again! Maybe I understand your point.
Great.
>As far as I read the code, in the standby mode, the startup process
>periodically checks the trigger file in WaitForWALToBecomeAvailable().
>No?
Yes it is.
>There can be small delay between the creation of the trigger
>This happens because the startup process detects the trigger file
>after pg_wal_replay_pause() succeeds, and then make the recovery
>get out of the paused state.
Yes that is.
>It might be problematic to end the paused
>state silently? So, to make the situation less confusing, what about
>Some comments about the set_bit/get_bit parts.
>I'm reading long_bytea_string_bug_fix_ver6_pg10.patch, but that
>applies probably to the other files meant for the existing releases
>(I think you could get away with only one patch for backpatching
>and one patch for v13, and committers will sort
>I think the second argument indicates the bit position, which would be max
>bytea length * 8. If max bytea length covers whole int32, the second argument
>>needs to be wider i.e. int64.
Yes, it makes sence and followed.
>I think we need a similar change in byteaGetBit() and byteaSetBit() as
I want to resent the mail, because last one is in wrong format and hardly to
read.
In addition, I think 'Size' or 'size_t' is rely on platform, they may can't
work on 32bit
system. So I choose 'int64' after ashutosh's review.
>>I think the second argument indicates the bit position, which
>> When I test the patch, I find an issue: I start a stream with
>> 'promote_trigger_file'
>> GUC valid, and exec pg_wal_replay_pause() during recovery and as below it
>> shows success to pause at the first time. I think it use a initialize
>> 'SharedPromoteIsTriggered' value first time I exec
>But, sorry,,, I failed to understand the issue that you reported, yet...
>You mean that the first call of pg_wal_replay_pause() in the step #2
>should check whether the trigger file exists or not? If so, could you
>tell me why we should do that?
Sorry about my pool english. The
Hello hackers,
I find a small issue in XLogInsertRecord() function: it checks whether it's able
to insert a wal by XLogInsertAllowed(), when refused it reports an error "cannot
make new WAL entries during recovery".
I notice that XLogInsertAllowed() reject to insert a wal record not only
>So I'd like to propose the attached patch. The patch changes the message
>logged when a promotion is requested, based on whether the recovery is
>in paused state or not.
It is a compromise, we should notice it in document I think.
Regards,
Highgo Software (Canada/China/Pakistan)
URL :
>I don't think this has really solved the overflow hazards. For example,
>in binary_encode we've got
>resultlen = enc->encode_len(VARDATA_ANY(data), datalen);
>result = palloc(VARHDRSZ + resultlen);
>and all you've done about that is changed resultlen from int to int64.
>On a 64-bit machine,
>Sorry about that, attached is the changed patch for PG13, and the one
>for older branches will send sooner.
A little update for the patch, and patches for all stable avilable.
Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
>I have not thought about this matter, but it seems to me that you
>should add this patch to the upcoming commit fest for evaluation:
>https://commitfest.postgresql.org/28/
Thanks.
I think about it more detailed, and find it's better to show the 'roident'
other than 'roname'. Because an old
Hello hackers,
I am researching about 'origin' in PostgreSQL, mainly it used in logical
decoding to filter transaction from non-local source. I notice that the
'origin' is stored in commit_ts so that I think we are possible to get 'origin'
of a transaction from commit_ts.
But I can not fond any
Hello thanks for the detailed review,
>I think the second argument indicates the bit position, which would be max
>bytea length * 8. If max bytea length covers whole int32, the second argument
>>needs to be wider i.e. int64.
Yes, it makes sence and followed.
> Some more comments on the patch
Thanks for all the suggestions.
>Yeah. In its current shape, it means that only pg_waldump would be
>able to know this information. If you make this information part of
>xlogdesc.c, any consumer of the WAL record descriptions would be able
>to show this information, so it would provide a
Thanks for reply.
>If you wish to add more information about a XLOG_SWITCH record, I
>don't think that changing the signature of XLogDumpRecordLen() is
>adapted because the record length of this record is defined as
>Horiguchi-san mentioned upthread, and the meaning of junk_len is
>confusing
Thanks for all the suggestion, and new patch attached.
>Andres suggested that we don't need that description with per-record
>basis. Do you have a reason to do that? (For clarity, I'm not
>suggesting that you should reving it.)
I think Andres is saying if we just log it in xlog_desc() then we
>It looks to me "We can know that length by subtracting the LSN of
>XLOG_SWITCH from the next record's LSN so it doesn't add any
>information."
Sorry,maybe I miss this before.
But I think it will be better to show it clearly.
>So the length of in this case is:
>
>LOC(SEG A+1) - ReadRecPtr -
>I think that the length of the XLOG_SWITCH record is no other than 24
>bytes. Just adding the padding? garbage bytes to that length doesn't
>seem the right thing to me.
>
>If we want pg_waldump to show that length somewhere, it could be shown
>at the end of that record explicitly:
>
>rmgr: XLOG
Hello hackers,
We know that pg_waldump can statistics size for every kind of records. When I
use
the feature I find it misses some size for XLOG_SWITCH records. When a user does
a pg_wal_switch(), then postgres will discard the remaining size in the current
wal
segment, and the pg_waldump tool
Hello hackers,
Currently, if BEFORE TRIGGER causes a partition change, it reports an error
'moving row
to another partition during a BEFORE FOR EACH ROW trigger is not supported' and
fails
to execute. I want to try to address this limitation and have made an initial
patch to get
feedback from
Hello,
I am trying to handle the limit that we can't do a tuple move caused by BEFORE
TRIGGER,
during which I get two doubt points:
The first issue:
In ExecBRUpdateTriggers() or ExecBRInsertTriggers() function why we need to
check the
result slot after every trigger call. If we should check
>I find this is the most latest mail with an attachment, so I test and reply on
>this thread, several points as below:
>1. I notice it produces new relfilenode when new session login and some
>data insert. But the relfilenode column in pg_class still the one when create
>the global temp table. I
>Fixed in global_temporary_table_v29-pg13.patch
>Please check.
I find this is the most latest mail with an attachment, so I test and reply on
this thread, several points as below:
1. I notice it produces new relfilenode when new session login and some
data insert. But the relfilenode column in
>> A second thing is that TransactionIdGetCommitTsData() was introdued in
>> core(73c986add). It has only one caller pg_xact_commit_timestamp() which
>> passes RepOriginId as NULL, making last argument to the
>> TransactionIdGetCommitTsData() a dead code in core.
>>
>> Quick code search shows
>The rationale for this interface is unclear to me. Please explain what
>happens in each case?
>In my proposal, we'd have:
>* Bad value, no --force:
> - program raises error, no work done.
>* Bad value with --force:
> - program raises warning but changes anyway.
>* Good value, no --force:
> -
>Cool, thanks. I have gone through your patch in details, and updated
>it as the attached. Here are some comments.
>'8000' as OID for the new function was not really random, so to be
>fair with the other patches, I picked up the first random value
>unused_oids has given me instead.
>
>There
>Regarding the attribute name, I was actually considering to just use
>"roident" instead. This is more consistent with pglogical, and that's
>also the field name we use in ReplicationState[OnDisk]. What do you
>think?
Yes that's is the right way, I can see it's 'roident' in
>+SELECT pg_replication_origin_create('test_commit_ts: get_origin_1');
>+SELECT pg_replication_origin_create('test_commit_ts: get_origin_2');
>+SELECT pg_replication_origin_create('test_commit_ts: get_origin_3');
>
>Why do you need three replication origins to test three times the same
>pattern?
>Thanks. Movead, please note that the patch is waiting on author?
>Could you send an update if you think that those changes make sense?
I make a patch as Michael Paquier described that use a new function to
return transactionid and origin, and I add a origin version to
pg_last_committed_xact()
Hello Andrey
>> I have researched your patch which is so great, in the patch only data
>> out of 'global_snapshot_defer_time' can be vacuum, and it keep dead
>> tuple even if no snapshot import at all,right?
>>
>> I am thanking about a way if we can start remain dead tuple just before
>> we
>I have doubts that I fully understood your question, but still.
>What real problems do you see here? Transaction t1 doesn't get state of
>shard2 until time at node with shard2 won't reach start time of t1.
>If transaction, that inserted B wants to know about it position in time
>relatively to t1
>When checking each tuple visibility, we always have to get the CSN
>corresponding to XMIN or XMAX from CSN SLRU. In the past discussion,
>there was the suggestion that CSN should be stored in the tuple header
>or somewhere (like hint bit) to avoid the overhead by very frequehntly
>lookup for CSN
>> ISTM that a reasonable compromise is that if you use -x (or -c, -m, -O)
>> and the input value is outside the range supported by existing files,
>> then it's a fatal error; unless you use --force, which turns it into
>> just a warning.
>One potential problem is that you might be using
>There is a conflict in catversion.h. If you wish to test the patch,
>please feel free to use the attached where I have updated the
>attribute name to roident.
I think everything is ok, but be careful the new patch is in Windows
format now.
Regards,
Highgo Software (Canada/China/Pakistan)
>> but be careful the new patch is in Windows format now.
>That would be surprising. Why do you think that?
I am not sure why, I can not apply your patch, and I open it
with vscode and shows a CRLF format, if I change the patch to
LF then nothing wrong.
Regards,
Highgo Software
>> I am not sure why, I can not apply your patch, and I open it
>> with vscode and shows a CRLF format, if I change the patch to
>> LF then nothing wrong.
>This looks like an issue in your environment, like with git's autocrlf
>or such? rep-origin-superuser-v7.patch has no CRLF.
Yes thanks,
>> would like to know if the patch related to CSN based snapshot [2] is a
>> precursor for this, if not, then is it any way related to this patch
>> because I see the latest reply on that thread [2] which says it is an
>> infrastructure of sharding feature but I don't understand completely
>>
>Yeah, the normal workaround is to create the necessary file manually in
>order to let the system start after such an operation; they are
>sometimes necessary to enable testing weird cases with wraparound and
>such. So a total rejection to work for these cases would be unhelpful
>precisely for
hello hackers,
When I try to use pg_resetwal tool to skip some transaction ID, I get a problem
that is
the tool can accept all transaction id I offered with '-x' option, however, the
database
may failed to restart because of can not read file under $PGDATA/pg_xact. For
example, the 'NextXID'
Thanks for reply.
>Probably it's not time to do the code review yet, but when I glanced the patch,
>I came up with one question.
>0002 patch changes GenerateCSN() so that it generates CSN-related WAL records
>(and inserts it into WAL buffers). Which means that new WAL record is generated
>You mean that the last generated CSN needs to be WAL-logged because any smaller
>CSN than the last one should not be reused after crash recovery. Right?
Yes that's it.
>If right, that WAL-logging seems not necessary because CSN mechanism assumes
>CSN is increased monotonically. IOW, even
Thanks for reply.
>AFAIU, this patch is to improve scalability and also will be helpful
>for Global Snapshots stuff, is that right? If so, how much
>performance/scalability benefit this patch will have after Andres's
>recent work on scalability [1]?
The patch focus on to be an infrastructure of
Hello hackers,
Currently, I do some changes based on the last version:
1. Catch up to the current commit (c2bd1fec32ab54).
2. Add regression and document.
3. Add support to switch from xid-base snapshot to csn-base snapshot,
and the same with standby side.
Regards,
Highgo Software
+{ oid => '2228', descr => 'split delimited text',
+ proname => 'string_to_table', prorows => '1000', proretset => 't',
+ prorettype => 'text', proargtypes => 'text text',
+ prosrc => 'text_to_table' },
+{ oid => '2282', descr => 'split delimited text with null string',
+ proname =>
>It may be OK actually; if you're doing multiple dangerous changes, you'd
>use --dry-run beforehand ... No? (It's what *I* would do, for sure.)
>Which in turns suggests that it would good to ensure that --dry-run
>*also* emits a warning (not an error, so that any other warnings can
>also be
>That was fast, thanks. I have not tested the patch, but there are
>two things I missed a couple of hours back. Why do you need
>pg_last_committed_xact_with_origin() to begin with? Wouldn't it be
>more simple to just add a new column to pg_last_committed_xact() for
>the replication origin?
>ISTM that a reasonable compromise is that if you use -x (or -c, -m, -O)
>and the input value is outside the range supported by existing files,
>then it's a fatal error; unless you use --force, which turns it into
>just a warning.
I do not think '--force' is a good choice, so I add a '--test,
I test the patch and occur several issues as blow:
Issue one:
Get a Assert error at 'Assert(bms_is_member(i, node->as_needrequest));' in
ExecAppendAsyncRequest() function when I use more than two foreign table
on different foreign server.
I research the code and do such change then the Assert
Thanks for review, and sorry for reply so later.
>I reviewed the patch and found some problems.
>>+ if(startSegNo != endSegNo)
>>+ else if(record->ReadRecPtr / XLOG_BLCKSZ !=
>>+ if(rmid == RM_XLOG_ID && info == XLOG_SWITCH)
>>+ if(ri == RM_XLOG_ID)
>>+ if(info == XLOG_SWITCH)
>You need to put a
67 matches
Mail list logo
