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

sql/pgwire: consider implementing opcode style execution for forming pgwire messages #65959

Open
yuzefovich opened this issue Jun 1, 2021 · 1 comment
Labels
C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team
Projects

Comments

@yuzefovich
Copy link
Member

yuzefovich commented Jun 1, 2021

Extracted from here.

pkg/sql/pgwire/conn.go, line 1282 at r11 (raw file):

		c.msgBuilder.initMsg(pgwirebase.ServerMsgDataRow)
		c.msgBuilder.putInt16(width)
		for colIdx, col := range vecs {

Idea for later: this could be the perfect place for us to prototype the "opcode style" of execution, since it's such a simple, constrained codepath. I'm not even sure if it would help matters, but I know from profiles that the most expensive part of places like this where we revert to row-based execution is the type switching.

So, what if we made a simple little opcode runner, and pre-calculated the types that we will be running through over and over again on every row?

For example, at plantime, we'd run something like this:

opcodes = []
for i, type := types {
    fmtCode = formatCodes[i]
    opcodes = append(opcodes, getOpcode(type, fmtCode)
}

An "opcode" would be something like EncodeBinaryInt32 or EncodeTextDate.

Then, at runtime (aka right here), we'd run:

for i := 0; i < n; i++ {
    // get rowidx
    for _, opCode := range opCodes {
        switch opCode {
            case EncodeBinaryInt32:
                ...
        }
    }
}

Then again maybe we'd gain exactly nothing here - we'd be replacing a type switch with an int switch, but we'd keep the interface-to-ptr cast...

Maybe the true way to make all of this better would be to use more unsafe.Ptr stuff to avoid the expensive, checked interface type lookup and cast.

Jira issue: CRDB-7814

@yuzefovich yuzefovich added the C-investigation Further steps needed to qualify. C-label will change. label Jun 1, 2021
@yuzefovich yuzefovich added this to Triage in SQL Queries via automation Jun 1, 2021
@yuzefovich yuzefovich moved this from Triage to Backlog in SQL Queries Jun 1, 2021
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@mgartner mgartner moved this from Backlog to New Backlog in SQL Queries Feb 16, 2023
@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team
Projects
Status: Backlog
SQL Queries
New Backlog
Development

No branches or pull requests

3 participants