Skip to content

[Feature Request] Make helio_api.drop_indexes less weird #49

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

Open
AlekSi opened this issue Feb 1, 2025 · 4 comments
Open

[Feature Request] Make helio_api.drop_indexes less weird #49

AlekSi opened this issue Feb 1, 2025 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@AlekSi
Copy link
Contributor

AlekSi commented Feb 1, 2025

Purpose of the feature.

helio_api.drop_indexes is the only procedure with the INOUT parameter. All other functions are… functions. That creates problems for us: FerretDB/FerretDB#4730. This parameter seems to return BSON in the BSONHEX text representation?.. Or something. In any case, having a single function (that is not a function) that stands out that much is bad for API consistency.

It would be nice to have it as a regular function we could use.

Describe the solution you'd like

There is a "normal" function to drop an index.

Describe alternatives you've considered

Add more hacks to support this procedure.

Additional context

N/A.

@jayanta-mondal
Copy link
Contributor

jayanta-mondal commented Feb 6, 2025

@AlekSi Thanks Alexey for bring this to our attention.

Just to understand your requirement, can you tell us a bit more about how are you using or the intended use of the drop_index API?

As you know FUNCTIONs can't start/commit/roll back transactions from within, and we need that capability in drop index, which involves marking the index being deleted and committing that information before proceeding with the delete. So we chose PROCEDURE organically.

We are looking into the output type/formatting issue.

@AlekSi
Copy link
Contributor Author

AlekSi commented Feb 12, 2025

As you know FUNCTIONs can't start/commit/roll back transactions from within

I did not actually know that. 😬

But why the last parameter is INOUT instead of just OUT, then?

After trying all possible ways, I still can't get the output as binary BSON instead of BSONHEX. PostgreSQL just does not provide a syntax for using type conversions in procedure calls, I think?

@visridha
Copy link
Contributor

OUT params for procedures must still be added in the CALL (just put in as NULL). INOUT can have default values which means it makes the CALL less weird as you don't need to specify the out params in the CALL.

@AlekSi
Copy link
Contributor Author

AlekSi commented Apr 30, 2025

That's how we call this procedure (see FerretDB/FerretDB#5114):

// DropIndexes is a wrapper for
//
//	documentdb_api.drop_indexes(p_database_name text, p_arg documentdb_core.bson, INOUT retval documentdb_core.bson DEFAULT NULL).
func DropIndexes(ctx context.Context, conn *pgx.Conn, l *slog.Logger, databaseName string, arg wirebson.RawDocument) (outRetVal wirebson.RawDocument, err error) {
	row := conn.QueryRow(ctx, "CALL documentdb_api.drop_indexes($1, $2::bytea)", databaseName, arg)

	var res []byte
	if err = row.Scan(&res); err != nil {
		err = mongoerrors.Make(ctx, err, "documentdb_api.drop_indexes", l)
	}

	spew.Dump(res)
	outRetVal = res

	return
}

Note how the second argument uses ::bytea to perform a type conversion. Without it, PostgreSQL assumes that the raw (binary) BSON we pass should be interpreted as text JSON and fails with:

"invalid input syntax JSON for BSON: Code: '1', Message 'Got parse error at \"\\\", position 0: \"ESCAPE_OUTSIDE_STRING\"'"

Similarly, the output is returned as text BSON (BSONHEX):

([]uint8) (len=69 cap=69) {
 00000000  42 53 4f 4e 48 45 58 31  66 30 30 30 30 30 30 30  |BSONHEX1f0000000|
 00000010  38 36 66 36 62 30 30 30  31 31 32 36 65 34 39 36  |86f6b0001126e496|
 00000020  65 36 34 36 35 37 38 36  35 37 33 35 37 36 31 37  |e646578657357617|
 00000030  33 30 30 30 32 30 30 30  30 30 30 30 30 30 30 30  |3000200000000000|
 00000040  30 30 30 30 30                                    |00000|
}

With functions, we use something like SELECT retval::bytea FROM xxx, using the same ::bytea conversion on the result. But there is no way to do that for procedures.

My understanding is that all this happens because BSON's desired format is text. Can it be made binary with some feature toggle, for example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants