Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert float8 with double instead of long double #826

Merged
merged 15 commits into from
Jan 2, 2025

Conversation

avamingli
Copy link
Contributor

Fixes #ISSUE_Number

What does this PR do?

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Breaking Changes

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions


darthunix and others added 14 commits December 30, 2024 23:52
It seems that GPDB faced a glibc bug on RedHat 6 with denormalized
numbers and fixed it using two-step conversion in `float8in()`:

- text to long double
- long double to double after denormalized numbers validation

This differs from PG approach where we rely on libc and make the
direct conversion to double. RedHat 6 seems to be an old system and
there is no much sense to differ from upstream and have additional
performance penalty converting long double to double in a two-step
manner and making additional checks.

There is another problem with this code - long double can have
problems on some architectures that are currently not supported by
GPDB, but where it can be ported in the future. Originally this
problem was detected as research about porting GPDB to PowerPC: it
doesn't have analogue of Intel 80-bit long double with 63-bit mantissa
and extended range between 1.0E-4932W and 1.0E+4932W.  Instead,
PowerPC has an extended double type combined from two doubles with
106-bit mantissa and the same range as double provides 1.0E−323L -
1.0E+308L. As a result, we can't use the same validation logic for the
numbers near +/- infinity as it is possible to do on Intel. So this
GPDB specific code doesn't help us to solve the problem on RedHat 6 on
PowerPC and fails the float test.

So, let's get rid of it and return the code to PG approach with a
double. Align float.c to upstream version at commit 9e1c9f till where
we merged to main branch so far.

Co-authored-by: Ashwin Agrawal <aashwin@vmware.com>
Reviewed-by: Alexandra Wang <walexandra@vmware.com>
The table gpexpand.status_detail used by gpexpand should be
distributed by "table_oid", to avoid the updating workload burden of
tens thounsand tables in a segment, concentrated on only one single
segment with the prior distribution policy (distributed by dbname).

Authored-by: Peng Han <phan@vmware.com>
The greenplum_clients_path.sh could not be sourced in other directories
before, copy greenplum_path.sh's codes to fix it.

```
$ cat /usr/local/greenplum-clients-devel/greenplum_clients_path.sh
GPHOME_CLIENTS=`pwd`
PATH=${GPHOME_CLIENTS}/bin:${GPHOME_CLIENTS}/ext/python/bin:${PATH}
...

$ set -x

$ . /usr/local/greenplum-clients-devel/greenplum_clients_path.sh
+ . /usr/local/greenplum-clients-devel/greenplum_clients_path.sh
+++ pwd
++ GPHOME_CLIENTS=/tmp/build/b251a50d
++ PATH=/tmp/build/b251a50d/bin:/tmp/build/b251a50d/ext/python/bin:...
...
```
The truncate_gp case lists the on-disk files to check the behavior of
truncate, but it had two issues that made the case flaky.

1, the length of filenames may be long enough to have a different output
title.
2, Python's `os.listdir()` doesn't guarantee the order.

This commit fixes it.
Currently, when doing an incremental recovery in verbose mode
with "gprecoverseg -v", we record the pg_controldata output of
only source data directory. We can also record the pg_controldata
output of the target data directory before running pg_rewind.

Context: Recently there have been multiple pg_rewind failures
due to missing wal files with customers. Often times at the
time of performing incremental recovery(pg_rewind), the target
segments have been shutdown for quite sometime, and if the
incremental recovery failed, users usually choose to perform a
full recovery as a workaround to bring the target segments up.
This sequence of operations would erase the clue of what had
happened on the target segment before it went down in the first
place. Recording the pg_controldata output of the target data
directory before running pg_rewind would tell us when and what
was the latest checkpoint on the target segment before it went
down, and that would be very helpful for future investigations.

Fixes https://github.com/greenplum-db/gpdb/issues/14627
…" when log_statement='ddl'.

The `LogStmtLevel GetCommandLogLevel(Node *parsetree)` function
lacks `NodeTag` handling for resource management commands.
As #14645 reported, the parallel retrieve cursor implements
communication between backends based on a shared memory message queue.
QE backend and retrieve backend share a typemod registry just like the
leader and worker of parallel query. However, unlike parallel query,
the retrieve backend reused by different endpoints. But after switching
the endpoint, it needs to detach from old DSM segment, and the local
record cache will also become invalid because it only saves pointers to
the old DSM segment.
Fixes #10803

Issue:
https://github.com/greenplum-db/gpdb/issues/10803
When ORCA generates a RESULT node with a LIMIT parent, the query returns a
subset of the actual result.

Root cause:
When ORCA generates a RESULT node with a LIMIT parent, the parent node's tuple
bound is pushed down to the RESULT node's child node. This is expected unless
the RESULT node has a filter. The filter is supposed to be applied before the
tuple bound, instead it's currently applied after the tuple bound. This leads to
the query returning a subset of the actual result.

Example:
Consider this plan shape --

->  Limit
	 ->  Result
	       Filter: (SubPlan 1)
	       ->  Sort
		     ...
	       SubPlan 1
		     ...

The RESULT node has a LIMIT parent and a SORT child. The correct procedure
should be: (1) sort the relation, (2) apply the filter to the sorted relation
and (3) apply limit to the filtered relation. But instead, the limit was applied
before the filter to the relation.

Solution:
When executing the plan, we allow tuple bound push down only if the RESULT node
DOES NOT has a filter condition. If the RESULT node has a non-NULL QUAL, we
don't apply the tuple bound to its child node.
* process encoding option for custom format

encoding option of custom format is lost on gpdb7 for external table.
However, we should process it to copystate to make sure non-UTF8 encoded file could be handled by custom format.
If a mirror is stopped immediately by SIGQUIT or SIGKILL, its state
will be DB_IN_ARCHIVE_RECOVERY. During startup of mirror (essentially
crash recovery aspect) may spend some time to SyncAllXLogFiles() or
StartupReplicationSlots() (which has to read stuff from disk) and such
methods before able to initialize xlogctl->RedoRecPtr. During this
window, coordinator fts process may send promotion request to this
mirror. ftsmessage-handler will try to create internal replication
slot, but ReplicationSlotReserveWal() call hangs in loop because the
shared XLogCtl structure is not initialized yet by startup process.

When the startup process calls
  ReplicationSlotDropIfExists(INTERNAL_WAL_REPLICATION_SLOT_NAME);
  FATAL 'replication slot "internal_wal_replication_slot" is already
  active' reports and then startup process exits. It can cause FTS
  double fault.

So, avoid this issue by judging GetRedoRecPtr() != InvalidXlogRecPtr
during promotion message ftsmessage-handler by FTS. If promotion
request is received before xlogctl->RedoRecPtr is initialized then
will ignore and let FTS retry the promotion request.

Writing test is extremely difficult for this scenario as need to pause
mirror during crash recovery before a particular point. Current fault
injector framework based on shared-memory doesn't help here as need
something that can persist reboot. Even thought about using
RECOVERY_TARGET_ACTION_PAUSE though that point kicks-in much later, we
want to pause the recovery before StartupXLOG() is able to set
xlogctl->RedoRecPtr.
https://github.com/greenplum-db/gpdb/pull/14650 moved the
distributed_snapshot test to input/output folder but missed
adding the .gitignore entries so the generated .sql and .out
files would appear in untracked files. Adding now.
Code adjustment in memtuple_form_to() to replace memset() to
combination with palloc0() in condition to reduce memset()
calls for performance improvemnt in bulk inserts cases.
ALTER DEFAULT PRIVILEGES IN SCHEMA ... affects all future tables
created in the same schema. Since the test 'partition' is running
in parallel with other tests like 'partition1', when some table is
created in 'partition1' test after the SELECT privilege on 'public'
is granted to a role in 'partition' test, that role cannot be dropped
later because that table still exists in another test, e.g.:

	 DROP ROLE user_prt_acl;
	+DETAIL:  privileges for table start_exclusive_smallint
	+ERROR:  role "user_prt_acl" cannot be dropped because some objects depend on it
	+privileges for table start_exclusive_smallint_1_prt_p1
	+privileges for table start_exclusive_smallint_1_prt_pmax

Fixing it by using a dedicated schema for the ALTER DEFAULT PRIVILEGES
test in 'partition'. Also leaving the table, schema and role not dropped
in order to test upgrade.
@avamingli avamingli added the cherry-pick cherry-pick upstream commts label Dec 30, 2024
my-ship-it
my-ship-it previously approved these changes Dec 31, 2024
Authored-by: Zhang Mingli avamingli@gmail.com
Copy link
Contributor

@my-ship-it my-ship-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@avamingli avamingli merged commit a12d2d4 into apache:main Jan 2, 2025
22 checks passed
@avamingli avamingli deleted the cp_2 branch January 2, 2025 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick cherry-pick upstream commts
Projects
None yet
Development

Successfully merging this pull request may close these issues.