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.

Sign up for GitHub

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

Jump to bottom

1.2 dev #16688

Closed
wants to merge 172 commits into from
Closed

1.2 dev #16688

wants to merge 172 commits into from

Conversation

aylei
Copy link
Contributor

@aylei aylei commented Jun 5, 2024 •
edited by codiumai-pr-agent-pro bot
Loading

User description

PR Type

Enhancement, Bug fix

Description

  • Added CheckOrphan method and related request/response types in lock.pb.go.
  • Cleaned up imports and updated condition check in serialize.go.
  • Refactored aggregation implementation and removed acceptNull in register.go.
  • Removed unused imports and fixed a typo in the comment in server.go.
  • Updated ants pool initialization to use runtime.GOMAXPROCS(0) in txnmgr.go.

Changes walkthrough 📝

Relevant files
Enhancement
lock.pb.go
Add CheckOrphan method and related request/response types

pkg/pb/lock/lock.pb.go

  • Added CheckOrphan method to the Method enum.
  • Introduced CheckOrphanRequest and CheckOrphanResponse types.
  • Updated Request and Response structs to include CheckOrphan.
  • Added methods to marshal and unmarshal CheckOrphanRequest and
    CheckOrphanResponse.
    		•  +853/-133
    serialize.go
    Clean up imports and update condition check                           

    pkg/sql/colexec/aggexec/serialize.go

  • Removed unused imports.
  • Updated condition to check encoded.Info.Id instead of
    encoded.GetExecType().
    		•  +1/-420 
    register.go
    Refactor aggregation implementation and remove acceptNull

    pkg/sql/colexec/aggexec/register.go

  • Refactored aggImplementation to separate context and logic
    implementations.
  • Removed acceptNull from SingleColumnAggInformation.
    		•  +19/-196
    server.go
    Remove unused imports and fix typo in comment                       

    pkg/frontend/server.go

  • Removed unused imports.
  • Fixed a typo in the BaseService interface comment.
    		•  +2/-35   
    txnmgr.go
    Update ants pool initialization to use GOMAXPROCS               

    pkg/vm/engine/tae/txn/txnbase/txnmgr.go

  • Changed ants.NewPool parameter to use runtime.GOMAXPROCS(0) instead of
    runtime.NumCPU().
    		•  +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    xzxiong and others added 30 commits May 10, 2024 08:47
    @xzxiong
    fix Bvt case in issue 14836 in 1.2 (matrixorigin#15966)
    22d81a3 
    ref matrixorigin#15914
changes:
1. move op `check statement_info result` to the end of bvt.

Approved by: @heni02, @sukki37
    @aptend
    cp catalog and options change to 1.2 (matrixorigin#15970)
    360a3a5 
    cp pr 1b00a08 and fb8da32

Approved by: @XuPeng-SH, @sukki37
    @xzxiong
    Support mo_logger disable ErrorCollection in 1.2 (matrixorigin#15978)
    a8d2879 
    ref: matrixorigin#15955
changes:
1. add new config `observability:disableError` default: false.
2. bugfix: if NoReportFromContext(ctx), ignore error log output and collection.

Approved by: @zhangxu19830126, @daviszhen, @sukki37
    @w-zr
    fix index table merge too much on 1.2 (matrixorigin#15981)
    90f8d5d 
    add target object size config when merge objects.

Approved by: @XuPeng-SH, @sukki37, @aptend
    @qingxinhome
    Predefine system table as global var (matrixorigin#15925) (matrixorig…
    fcc6dee 
    …in#15987)

把mo系统表和系统视图声明为全局变量，都引用自一处，降低耦合

Approved by: @daviszhen, @zhangxu19830126, @sukki37
    @ouyuanning
    fix bug: incorrect row count for value scan node (matrixorigin#15941) (…
    2b64e74 
    …matrixorigin#15989)

fix bug: incorrect row count for value scan node

Approved by: @aunjgr, @badboynt1, @sukki37
    @LeftHandCold
    Fix the bug of losing deltaLoc when doing checkpoint for 1.2 (matrixo…
    71f6c38 
    …rigin#15993)
    @badboynt1
    force index join on one cn (matrixorigin#15990)
    c79fd88 
    修复一个bug导致没有完全改过来的情况，需要让整颗子树都强制走单cn

Approved by: @aunjgr, @heni02, @qingxinhome, @sukki37
    @daviszhen
    fix drop database failure (matrixorigin#16000)
    1760453 
    问题原因：
在drop database 会从mo_table_partitions删除索引信息。过程中会加锁。
加锁过程中，会用tableId 取relation。
取relation时，会遍历CatalogCache中的数据库。但是同一个事务中的数据库已经被删除了。
导致engine.Database取Database对象时，报错。

修改方案：
事务中删除drop database时，会先存入deletedDatabaseMap。
在遍历catalogCache中的数据库时，将已经加入deletedDatabaseMap的数据库名称排除掉。避免报错。


ci regression: https://github.com/matrixorigin/ci-test/actions/runs/9032264260

Approved by: @triump2020, @sukki37
    @jiangxinmeng1
    fix HasCommittedPersistedData, cherry-pick matrixorigin#15930 (matrix…
    b5c2eaa 
    …origin#15965)

fix HasCommittedPersistedData, cherry-pick matrixorigin#15930

Approved by: @XuPeng-SH, @sukki37
    @jensenojs
    [cherry-pick-15957] : let preinsert can get outcnt from stats (matrix…
    60cdf18 
    …origin#16004)

实现 matrixorigin#15933 的子任务

Approved by: @ouyuanning, @m-schen, @sukki37
    @qingxinhome
    Restore bootstrap to establish index code for mo-task (matrixorigin#1…
    435d1d2 
    …6012)

把mo系统表和系统视图声明为全局变量时，误删mo_task库下的索引定义

Approved by: @zhangxu19830126, @daviszhen, @sukki37
    @daviszhen
    fix hung (matrixorigin#16015)
    c747855 
    问题原因：
在发送列定义时，用getStatusWithTxnEnd取server status了。
对getStatusWithTxnEnd 使用有错误。只能在事务结束后才能用。在事务中使用时，proxy会把session迁移走，导致没数据响应client。而卡住。

修改方案：
在发送列定义时，改用handler.GetServerStatus取server status

Approved by: @qingxinhome, @sukki37
    @w-zr
    add error log in taskservice (matrixorigin#16018)
    bb20ff7 
    Add error log in taskservice when task service cannot get db on 1.2-dev.

Approved by: @sukki37, @zhangxu19830126
    @volgariver6
    [bug] fix ut TestAddressManager (matrixorigin#16003)
    e2abc17 
    fix ut TestAddressManager

Approved by: @zhangxu19830126, @sukki37
    @zhangxu19830126
    improve incr service to 1.2 (matrixorigin#16020)
    cbd5b7c 
    improve incr service  to 1.2

Approved by: @m-schen, @sukki37
    @YANGGMM
    add quote to dbname when drop account (matrixorigin#1.2-dev) (matrixo…
    006a6bf 
    …rigin#16024)

add quote to dbname when drop account

Approved by: @heni02, @qingxinhome, @sukki37, @daviszhen
    @gouhongshen
    sync to 1.2: fix ForceCheckpoint retry logic. (matrixorigin#16030)
    38998dc 
    should re-clear the pending ckp when retry.

Approved by: @XuPeng-SH, @sukki37
    @aunjgr
    [cherry-pick] fix argument type of prefix_in (matrixorigin#16023)
    aa01312 
    change type of prefix_in's second argument from T_tuple to T_varchar

Approved by: @badboynt1, @sukki37
    @daviszhen
    add dump profile (matrixorigin#16026)
    784f1a7 
    解决OMM时，profile过时问题。
在收到TERM和INT信号时，保存heap和goroutine profile 到etlfs。

限制：
1.不能处理KILL信号。
2.写etlfs的超时为3分钟。写heap,goroutine总共最多6分钟。

Approved by: @reusee, @zhangxu19830126, @sukki37
    @daviszhen
    fix parse oom (matrixorigin#16041)
    7462a7c 
    本地sysbench 1000w 没复现出oom。

对2处潜在的stmt引用问题，做了修改。

Approved by: @qingxinhome, @sukki37
    @iamlinjunhong
    retry for first lock table bind change (matrixorigin#16036)
    7bc191e 
    retry for first lock table bind change

Approved by: @m-schen, @XuPeng-SH, @daviszhen, @zhangxu19830126, @reusee, @qingxinhome, @sukki37
    @YANGGMM
    Fix bug for snapshot-read(1.2-dev) (matrixorigin#16033)
    90a2947 
    Fix bug for snapshot-read

Approved by: @XuPeng-SH, @heni02, @reusee, @sukki37
    @reusee
    malloc: optimizations (matrixorigin#16037) (matrixorigin#16039)
    22194fc 
    malloc: refine cache size calculation

malloc: remove AllocTyped

malloc: remove eviction

malloc: add shard alloc and free statistics

malloc: flush cached objects when idle

Approved by: @zhangxu19830126, @sukki37
    @guguducken
    add leak config to mergerun cn for 1.2-dev (matrixorigin#16052)
    6a6d255
    @triump2020
    Fix bug for snapshot-read-cherry-pick (matrixorigin#15994) (matrixori…
    0080b6f 
    …gin#16051)

1. cherry pick

Approved by: @reusee, @XuPeng-SH, @sukki37
    @jiangxinmeng1
    fix test delete3, cherry-pick matrixorigin#16054 (matrixorigin#16056)
    7db16b3 
    fix test delete3, cherry-pick matrixorigin#16054

Approved by: @XuPeng-SH, @sukki37
    @daviszhen
    fix wrong txn status (matrixorigin#16071)
    9bb2626 
    1. 修改错误的迁移场景

begin，后无commit。
autocommit = 0且无begin且无commit。

这两种场景不应该有迁移。如果迁移后面会有问题。

2. 删除不必要的状态切换

ci regression : https://github.com/matrixorigin/ci-test/actions/runs/9062088432

Approved by: @qingxinhome, @sukki37
    @jiangxinmeng1
    Fix flush deletes, cherry-pick matrixorigin#15961 (matrixorigin#16060)
    9397437 
    Not check whether object is dropped when flush deletes.

Approved by: @XuPeng-SH, @sukki37
    @lcxznpy
    change use workflow branch (matrixorigin#16019)
    76ce4ef
    daviszhen and others added 4 commits June 5, 2024 04:07
    @daviszhen
    add debug info for panic (matrixorigin#16635)
    82a9d0b 
    issue上的问题是事务状态异常。

在出问题的调用栈上，增加事务状态的检测逻辑。

txnIsValid 判断事务状态是否异常。

Approved by: @m-schen, @triump2020, @qingxinhome, @XuPeng-SH, @ouyuanning, @aunjgr, @sukki37
    @qingxinhome
    Optimize logs (matrixorigin#16647)
    eefded8 
    delete unused logs for upgrader and sqlExecutor

Approved by: @daviszhen, @zhangxu19830126, @m-schen, @aunjgr, @sukki37
    @w-zr
    Support merge policy parameter 1.2 dev (matrixorigin#16669)
    0850ab6 
    support some policy parameters.

Approved by: @XuPeng-SH, @heni02, @sukki37
    @aylei
    fix: respsect filterFn in pluginRouter
    503646b 
    Signed-off-by: Aylei <[email protected]>
    @aylei aylei closed this Jun 5, 2024
    @mergify Mergify
    Copy link
    Contributor

    mergify bot commented Jun 5, 2024

    ⚠️ The sha of the head commit of this PR conflicts with #16689. Mergify cannot evaluate rules on this PR. ⚠️

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR includes a wide range of changes across multiple files and functionalities, including transaction management, aggregation functions, and system views. The complexity and potential impact of these changes require a thorough review to ensure compatibility, performance, and correctness.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Performance Issue: The change from runtime.NumCPU() to runtime.GOMAXPROCS(0) in pkg/vm/engine/tae/txn/txnbase/txnmgr.go might affect performance depending on the environment's configuration. It's crucial to evaluate if this change leads to optimal utilization of available CPUs.

    Code Consistency: The removal of imports in pkg/sql/colexec/aggexec/serialize.go without removing associated code that might use these imports could lead to compilation errors or undefined behaviors.

    🔒 Security concerns

    No

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Check for nil pointer dereference before accessing struct fields

    To avoid potential nil pointer dereference, consider checking if m is nil before calling
    GetByBegin and GetAutocommit methods.

    pkg/pb/txn/txn.pb.go [1823-1835]

     func (m *TxnOptions) GetByBegin() bool {
-	if m != nil {
-		return m.ByBegin
+	if m == nil {
+		return false
 	}
-	return false
+	return m.ByBegin
 }
 
 func (m *TxnOptions) GetAutocommit() bool {
-	if m != nil {
-		return m.Autocommit
+	if m == nil {
+		return false
 	}
-	return false
+	return m.Autocommit
 }
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies and fixes a potential nil pointer dereference, which is a critical issue that could lead to runtime panics.

    		10
    Add error handling for the proto.EnumName function to handle cases where the enum value is not found

    Add error handling for the proto.EnumName function in the String method of
    SessionLoggerInfo_LogLevel to handle cases where the enum value is not found.

    pkg/pb/pipeline/pipeline.pb.go [150-151]

     func (x SessionLoggerInfo_LogLevel) String() string {
-	return proto.EnumName(SessionLoggerInfo_LogLevel_name, int32(x))
+	if name, ok := SessionLoggerInfo_LogLevel_name[int32(x)]; ok {
+		return name
+	}
+	return "Unknown"
 }
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the proto.EnumName function in the String method of SessionLoggerInfo_LogLevel is crucial to prevent returning incorrect values, thus this suggestion significantly improves the code's reliability and correctness.

    		8
    Best practice
    Use ENUM type for columns storing boolean-like values to enforce data integrity

    Consider using ENUM('N','Y') instead of varchar(10) for columns that store boolean-like
    values such as 'N' and 'Y'. This can help enforce data integrity and reduce storage space.

    pkg/util/sysview/predefined.go [76-77]

    -Password_require_current varchar(10)  DEFAULT NULL,
-account_locked varchar(10)  NOT NULL DEFAULT 'N',
+Password_require_current ENUM('N','Y')  DEFAULT NULL,
+account_locked ENUM('N','Y')  NOT NULL DEFAULT 'N',
    Suggestion importance[1-10]: 8

    Why: The suggestion to use ENUM('N','Y') instead of varchar(10) for boolean-like values is valid and improves data integrity and storage efficiency. The lines and context in the PR match the suggestion.

    		8
    Add a default case to handle unexpected values in the SessionLoggerInfo_LogLevel_name and SessionLoggerInfo_LogLevel_value maps

    Add a default case to the SessionLoggerInfo_LogLevel_name and
    SessionLoggerInfo_LogLevel_value maps to handle unexpected values gracefully.

    pkg/pb/pipeline/pipeline.pb.go [132-148]

     var SessionLoggerInfo_LogLevel_name = map[int32]string{
 	0: "Debug",
 	1: "Info",
 	2: "Warn",
 	3: "Error",
 	4: "Panic",
 	5: "Fatal",
+	-1: "Unknown",
 }
 
 var SessionLoggerInfo_LogLevel_value = map[string]int32{
 	"Debug": 0,
 	"Info":  1,
 	"Warn":  2,
 	"Error": 3,
 	"Panic": 4,
 	"Fatal": 5,
+	"Unknown": -1,
 }
    Suggestion importance[1-10]: 7

    Why: Adding a default case to handle unexpected values in the maps is a good practice to avoid potential runtime errors, making this suggestion valuable for robustness.

    		7
    Rename the boolean field to follow Go naming conventions for better readability

    Consider renaming the ByBegin field to IsByBegin to make it clear that it represents a
    boolean flag. This improves readability and follows Go naming conventions for boolean
    variables.

    pkg/pb/txn/txn.pb.go [1692]

    -ByBegin              bool            `protobuf:"varint,14,opt,name=byBegin,proto3" json:"byBegin,omitempty"`
+IsByBegin              bool            `protobuf:"varint,14,opt,name=isByBegin,proto3" json:"isByBegin,omitempty"`
    Suggestion importance[1-10]: 7

    Why: Renaming 'ByBegin' to 'IsByBegin' enhances clarity for boolean variables, aligning with Go conventions. This is a good suggestion for readability but not critical.

    		7
    Enhancement
    Use a switch statement instead of multiple if statements to improve readability and maintainability

    Consider using a switch statement instead of multiple if statements in the GetLimit and
    GetOffset functions to improve readability and maintainability.

    pkg/pb/pipeline/pipeline.pb.go [3629-3644]

     func (m *Instruction) GetLimit() *plan.Expr {
-	if m != nil {
+	switch {
+	case m != nil:
 		return m.Limit
+	default:
+		return nil
 	}
-	return nil
 }
 
 func (m *Instruction) GetOffset() *plan.Expr {
-	if m != nil {
+	switch {
+	case m != nil:
 		return m.Offset
+	default:
+		return nil
 	}
-	return nil
 }
    Suggestion importance[1-10]: 6

    Why: The suggestion to replace if statements with a switch statement in GetLimit and GetOffset functions is valid for improving readability, but it's a minor enhancement rather than a crucial change.

    		6
    Use TEXT type for columns that may contain long text to handle data more efficiently

    Consider using TEXT instead of varchar(2048) for the DESCRIPTION column in the
    information_schema.CHARACTER_SETS table to handle longer descriptions more efficiently.

    pkg/util/sysview/predefined.go [247]

    -DESCRIPTION varchar(2048),
+DESCRIPTION TEXT,
    Suggestion importance[1-10]: 6

    Why: Changing 'varchar(2048)' to 'TEXT' for the 'DESCRIPTION' column in the CHARACTER_SETS table is a reasonable suggestion for handling potentially long descriptions more efficiently. However, the impact on performance and storage might be minimal given the specific use case.

    		6

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @zhangxu19830126 zhangxu19830126 Awaiting requested review from zhangxu19830126 zhangxu19830126 is a code owner

    @aunjgr aunjgr Awaiting requested review from aunjgr aunjgr is a code owner

    @m-schen m-schen Awaiting requested review from m-schen m-schen is a code owner

    @fengttt fengttt Awaiting requested review from fengttt fengttt is a code owner

    @volgariver6 volgariver6 Awaiting requested review from volgariver6 volgariver6 is a code owner

    @XuPeng-SH XuPeng-SH Awaiting requested review from XuPeng-SH XuPeng-SH is a code owner

    @LeftHandCold LeftHandCold Awaiting requested review from LeftHandCold LeftHandCold is a code owner

    @reusee reusee Awaiting requested review from reusee reusee is a code owner

    @triump2020 triump2020 Awaiting requested review from triump2020 triump2020 is a code owner

    @aptend aptend Awaiting requested review from aptend aptend is a code owner

    @xzxiong xzxiong Awaiting requested review from xzxiong xzxiong is a code owner

    @ouyuanning ouyuanning Awaiting requested review from ouyuanning ouyuanning is a code owner

    @badboynt1 badboynt1 Awaiting requested review from badboynt1 badboynt1 is a code owner

    @qingxinhome qingxinhome Awaiting requested review from qingxinhome qingxinhome is a code owner

    @iamlinjunhong iamlinjunhong Awaiting requested review from iamlinjunhong iamlinjunhong is a code owner

    @daviszhen daviszhen Awaiting requested review from daviszhen daviszhen is a code owner

    @aressu1985 aressu1985 Awaiting requested review from aressu1985 aressu1985 is a code owner

    @heni02 heni02 Awaiting requested review from heni02 heni02 is a code owner

    @sukki37 sukki37 Awaiting requested review from sukki37 sukki37 is a code owner

    Assignees
    No one assigned
    Labels
    Bug fix Enhancement Review effort [1-5]: 4
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.